[PATCH v3 09/17] Teach non-stop to do in-line step-overs (stop all, step, restart)
Pedro Alves
palves@redhat.com
Sat May 23 15:29:00 GMT 2015
On 05/22/2015 08:39 PM, Doug Evans wrote:
> Pedro Alves writes:
> > > Can you elaborate on how to interpret the name of this function?
> > > Guessing at how I'm supposed to interpret what this function is for,
> > > is a better name
> >
>> "breakpoints_should_have_been_inserted_by_now_or_should_remain_inserted"?
> > > [Not that that's my recommendation :-). Just trying to understand how
> > > to read this function.]
> >
> > You got it right, but I'm afraid I lack the English skills to come
> > up with a better name. "be inserted" is the state we want to reach, not
> > an action. Maybe "should_breakpoints_be_inserted_now" is little clearer,
> > but it still doesn't distinguish "state" vs "action". Because
> > state("be inserted"=false) is clearly "no breakpoints on target",
> > while action("be inserted"=false) could mean that whatever
> > breakpoint is already inserted remains inserted.
>
> I think I can manage with this change:
>
> - /* Don't remove breakpoints yet if, even though all threads are
> - stopped, we still have events to process. */
> + /* We still need breakpoints, even though all threads are
> + stopped, if we still have events to process. */
>
I like that. Thanks.
> But I'll submit that separately to not interfere with
> this patchset.
I don't mind folding that in at all. Just let me know.
For better names, your version of the comment made me think of:
breakpoints_should_be_on_target
breakpoints_should_be_on_target_now
need_breakpoint_on_target
breakpoints_needed_on_target
need_breakpoint_on_target_now
breakpoints_needed_on_target_now
want_breakpoints_on_target
breakpoints_wanted_on_target
want_breakpoints_on_target_now
breakpoints_wanted_on_target_now
Main difference is "be on target" instead of "be inserted".
The former seems less ambiguous to me.
>
> > > > + if (tp->suspend.waitstatus_pending_p)
> > > > + {
> > > > + if (debug_infrun)
> > > > + {
> > > > + char *statstr;
> > > > +
> > > > + statstr = target_waitstatus_to_string (&tp->suspend.waitstatus);
> > > > + fprintf_unfiltered (gdb_stdlog,
> > > > + "infrun: resume: thread %s has pending wait status %s "
> > > > + "(currently_stepping=%d).\n",
> > > > + target_pid_to_str (tp->ptid), statstr,
> > > > + currently_stepping (tp));
> > > > + xfree (statstr);
> > >
> > > Not something that has to be done with this patch of course,
> > > but it's nice that we don't have to track the memory of
> target_pid_to_str;
> > > IWBN to be able to do the same for target_waitstatus_to_string.
> > > [In C++ it could just return a string, and we *could* just wait for
> C++.
> > > Just a thought.]
> >
> > I'm just going with the flow you yourself created. ;-)
>
> What's the point of saying something like that?
Sorry, that was uncalled for. Too many late hours lead to
bad jokes. Please accept my apologies.
> I'm going to assume you don't disagree with the improvement.
I don't.
Thanks,
Pedro Alves
More information about the Gdb-patches
mailing list