[PATCH 2/2] Fix advance/until and multiple locations (PR gdb/26524)

[Simon Marchi]

On 2020-08-27 3:54 p.m., Pedro Alves wrote:
> On 8/26/20 10:01 PM, Simon Marchi wrote:
>> On 2020-08-22 4:38 p.m., Pedro Alves wrote:
> 
>>> -  /* Breakpoint set at the return address in the caller frame.  May be
>>> -     NULL.  */
>>> -  breakpoint_up caller_breakpoint;
>>> +  /* The breakpoint set at the return address in the caller frame,
>>> +     plus breakpoints at the destination location.  */
>>
>> I think it would make sense to use the plural "locations" in the comment, if we are
>> talking about multiple destinations locations in terms of machine code.  Though it
>> could be ok at the singular if you meant the source location, ofwhich there is only
>> one.
> 
> Thanks for spotting that.  It was a typo.  I've added "all" as well:
> 
>   /* The breakpoint set at the return address in the caller frame,
>      plus breakpoints at all the destination locations.  */
>   std::vector<breakpoint_up> breakpoints;
> 
>>> +  for (const breakpoint_up &bp : breakpoints)
>>> +    if (bpstat_find_breakpoint (tp->control.stop_bpstat,
>>> +				bp.get ()) != NULL)
>>> +      {
>>> +	set_finished ();
>>> +	break;
>>> +      }
>>
>> When using nested control structures, I thought we required braces for the
>> outer ones, like this (as mentioned in the GNU coding standards):
>>
>>   for (...)
>>     {
>>       if (...)
>>         ...
>>     }
>>
> 
> Hmm, where in the GNU coding standards?  I don't recall seeing that.
> 
> There's a rule for nested if-statements, at 
> <https://www.gnu.org/prep/standards/standards.html#Writing-C>:
> 
>   "When you have an if-else statement nested in another if statement, always put braces around the if-else. Thus, never write like this: "
> 
> That helps with avoiding confusion and the dangling else problem,
> though compilers warn about bad indentation nowadays catching
> it too.
> 
> But here I don't see the benefits.
> 
> I don't see it in https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards
> nor in the GCC standards either.
> 
> It's also frequently used in GDB:
> 
>  $ grep -rn "for (" gdb/ -A 1 | grep "if (" | wc -l
>  564
> 
> Here in the whole tree:
> 
>  $ grep -rn "for (" -A 1 | grep "if (" | wc -l
>  2472
> 
> And here in gcc, just under the gcc/ dir:
> 
>  $ grep -rn "for (" gcc/ -A 1 | grep "if (" | wc -l
>  6469                                                                                                                                                                        
> 
> You can see how they typically look with e.g.:
> 
>  $ grep -rn "for (" -A 1 | grep "if (" -B 1 | less
> 
>> If that's not the case, I have been misleading people for a while!
> 
> I'm afraid you may have!

I was indeed referring to the if-else thing in the standard.  By extension, I had assumed
it applied to all control structures.  If the standard explained the rationale (like the
Google coding standards do, for example), I wouldn't have made this mistake :(.

So it doesn't even apply if you just have two if.

  if (hello)
    if (bonjour)
      printf ("hola\n");

would be correct (though you could merge the two conditions with an && and have a single if...).

Simon