Re: [PATCH 2/4] gdb: make get_discrete_bounds return bool

[Simon Marchi via Gdb-patches <gdb-patches@sourceware.org>]

On 2020-12-06 1:03 a.m., Joel Brobecker wrote:
> Hi Simon,
> 
> On Mon, Nov 23, 2020 at 11:21:18AM -0500, Simon Marchi via Gdb-patches wrote:
>> get_discrete_bounds currently has three possible return values (see its
>> current doc for details).  It appears that for all callers, it would be
>> sufficient to have a boolean "worked" / "didn't work" return value.
>>
>> Change the return type of get_discrete_bounds to bool and adjust all
>> callers.  Doing so simplifies the following patch.
>>
>> gdb/ChangeLog:
>>
>> 	* gdbtypes.h (get_discrete_bounds): Return bool, adjust all
>> 	callers.
>> 	* gdbtypes.c (get_discrete_bounds): Return bool.
>>
>> Change-Id: Ie51feee23c75f0cd7939742604282d745db59172
> 
> A small reminder to remember to remove the Change-Id...
> 
> Other than that, the change looks good to me.
> 
> This is a very nice simplification of the interface, IMO, so thank you
> for doing that. In reading the documentation prior to this change,
> it was hard for me to wrap my head around what it the function was
> really doing.  In trying to understand the initial motivation, I did
> a bit of archeology, and it goes all the way back to the initial creation
> of the sourceware repository (20 years ago already!), so no obvious
> explanation from there.
> 
>> @@ -399,7 +399,7 @@ m2_language::value_print_inner (struct value *val, struct ui_file *stream,
>>   
>>   	  fputs_filtered ("{", stream);
>>   
>> -	  i = get_discrete_bounds (range, &low_bound, &high_bound);
>> +	  i = get_discrete_bounds (range, &low_bound, &high_bound) ? 0 : -1;
>>   	maybe_bad_bstring:
>>   	  if (i < 0)
>>   	    {
> 
> FTR, this hunk required a bit more context to investigate. We can see
> that the change looks correct when looking at how variable "i" is
> (mis)used:
> 
>            i = get_discrete_bounds (range, &low_bound, &high_bound) ? 0 : -1;
>          maybe_bad_bstring:
>            if (i < 0)
>              {
>                fputs_styled (_("<error value>"), metadata_style.style (),
>                              stream);
>                goto done;
>              }
> 
>            for (i = low_bound; i <= high_bound; i++)
> 
> Because of the use of labels, it's hard to propose a simpler rewriting
> which one would feel confident about without testing...

Indeed this code is really messy.  For the error handling of the 
value_bit_index, it just assigns `i` again and jumps back to 
maybe_bad_bstring, this is crazy.  It's really tempting to do more 
cleanup, but I prefer to just to the bare minimum changes in the Modula 
and Pascal code.

Simon

> 
> 
>> diff --git a/gdb/p-valprint.c b/gdb/p-valprint.c
>> index 428b2efc656..8f785b71ea4 100644
>> --- a/gdb/p-valprint.c
>> +++ b/gdb/p-valprint.c
>> @@ -343,7 +343,8 @@ pascal_value_print_inner (struct value *val, struct ui_file *stream,
>>   
>>   	  fputs_filtered ("[", stream);
>>   
>> -	  int bound_info = get_discrete_bounds (range, &low_bound, &high_bound);
>> +	  int bound_info = (get_discrete_bounds (range, &low_bound, &high_bound)
>> +			    ? 0 : -1);
>>   	  if (low_bound == 0 && high_bound == -1 && TYPE_LENGTH (type) > 0)
>>   	    {
>>   	      /* If we know the size of the set type, we can figure out the
> 
> Similar story here.
>