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

Simon Marchi via Gdb-patches
2020-12-07T15:19:34Z

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.
>