Re: [RFA] Show locno for 'multi location' breakpoint hit msg+conv var $bkptno $locno.

[Philippe Waroquiers <philippe.waroquiers@skynet.be>]

On Fri, 2022-06-03 at 13:40 -0700, Keith Seitz wrote:
> I've really only got some pretty minor questions to ask and a nit or
> two to mention. Nothing serious.
Thanks for the review.
> > > > Also, when a breakpoint is reached, the convenience variables
> > > > $bkptno and $locno are set to the encountered breakpoint number
> > > > and location number.
> 
> We have convenience variables for when we set breakpoints ($bpnum)
> and tracepoints ($tpnum). Should we re-use those or would that lead
> to user confusion? AFAICT, we only use those variables when a
> {break,trace}point is created or modified. This seems like it could
> be a natural extension of the utility of those variables.
> 
> I am not asking for any changes, I'm genuinely curious if reusing
> these existing convenience vars would be desirable in your opinion.
The drawback of re-using $bpnum is that this might break the logic of 
some existing scripts/command sequences, e.g. in the following case:
   add a breakpoint (so setting $bpnum)
   do whatever so that another breakpoint is encountered
   do whatever with $bpnum e.g. delete it or disable it.

If in the second step, $bpnum is changed, then the third step will change of
behaviour: instead of deleting or disabling the last set breakpoint,
it will delete or disable the last encountered breakpoint.

> 
> > > > +static bool
> > > > +has_multiple_locations (int num)
> 
> At one time, we used to append "_p" to these types of predicate-
> style functions. Are we abandoning this practice? [Mind you,
> this is simply a meta-question. Again, I am not asking for
> any changes.]
It looks to me that we now have a mix of functions with or without _p.
Maybe the _p convention was invented when bool functions were in fact returning an int ?

> > > > +	{
> > > > +	  warning (_("location number not found for breakpoint %d address %p."),
> > > > +		   b->number, paddress (bl->gdbarch, bl->address));
> 
> Doesn't paddress require %s (or even nicer: %ps with a styled_string ())?
The other usages of paddress use %s, I will fix this.

> > > >   #
> > > > @@ -699,7 +708,13 @@ proc runto { linespec args } {
> > > >   	    }
> > > >   	    return 1
> > > >   	}
> > > > -	-re "Breakpoint \[0-9\]*, \[0-9xa-f\]* in .*$gdb_prompt $" {
> > > > +	-re "Breakpoint \[0-9\]*, \[0-9xa-f\]* in .*$gdb_prompt $" {
> > > > +	    if { $print_pass } {
> > > > +		pass $test_name
> > > > +	    }
> > > > +	    return 1
> > > > +	}
> > > > +	-re "Breakpoint \[0-9\]*\.\[0-9\]*, \[0-9xa-f\]* in .*$gdb_prompt $" {
> > > >   	    if { $print_pass } {
> > > >   		pass $test_name
> > > >   	    }
> 
> It doesn't seem that we make a distinction here between the "Breakpoint N" vs "Breakpoint N.M"
> cases. Is it possible to merge these two branches into one using bkptno_numopt_re?
> [super super minor nit]
Will look at this.

> 
> Thank you so much for your patience. I hope a global maintainer with approval
> authority will follow-up.
Thanks for the review, I will prepare an RFA v2 handling your comments and the comments of
Eli.

Philippe