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

[Joel Brobecker <brobecker@adacore.com>]

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


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

-- 
Joel