Re: [PATCH v3 09/17] Teach non-stop to do in-line step-overs (stop all, step, restart)

[Pedro Alves <palves@redhat.com>]

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