Re: [PATCH 1/4] gdb: make discrete_position return optional

[Joel Brobecker <brobecker@adacore.com>]

Hi Simon,

On Mon, Nov 23, 2020 at 11:21:17AM -0500, Simon Marchi via Gdb-patches wrote:
> Instead of returning a boolean status and returning the value through a
> pointer, return an optional that does both jobs.  This helps in the
> following patches, and I think it is an improvement in general.
> 
> gdb/ChangeLog:
> 
> 	* ada-lang.c (ada_value_slice_from_ptr): Adjust.
> 	(ada_value_slice): Adjust.
> 	(pos_atr): Adjust.
> 	* gdbtypes.c (get_discrete_bounds): Adjust.
> 	(discrete_position): Return optional.
> 	* gdbtypes.h (discrete_position): Return optional.

Thanks for this patch; an interesting cleanup!

This patch LGTM.

> Change-Id: I758dbd8858b296ee472ed39ec35db1dbd624a5ae
> ---
>  gdb/ada-lang.c | 27 ++++++++++++++++-----------
>  gdb/gdbtypes.c | 33 ++++++++++++++++++++-------------
>  gdb/gdbtypes.h |  4 +++-
>  3 files changed, 39 insertions(+), 25 deletions(-)
> 
> diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
> index 714227d24dd..9ff470d97a7 100644
> --- a/gdb/ada-lang.c
> +++ b/gdb/ada-lang.c
> @@ -2817,11 +2817,13 @@ ada_value_slice_from_ptr (struct value *array_ptr, struct type *type,
>  			       type0->dyn_prop (DYN_PROP_BYTE_STRIDE),
>  			       TYPE_FIELD_BITSIZE (type0, 0));
>    int base_low =  ada_discrete_type_low_bound (type0->index_type ());
> -  LONGEST base_low_pos, low_pos;
> +  gdb::optional<LONGEST> base_low_pos, low_pos;
>    CORE_ADDR base;
>  
> -  if (!discrete_position (base_index_type, low, &low_pos)
> -      || !discrete_position (base_index_type, base_low, &base_low_pos))
> +  low_pos = discrete_position (base_index_type, low);
> +  base_low_pos = discrete_position (base_index_type, base_low);
> +
> +  if (!low_pos.has_value () || !base_low_pos.has_value ())
>      {
>        warning (_("unable to get positions in slice, use bounds instead"));
>        low_pos = low;
> @@ -2832,7 +2834,7 @@ ada_value_slice_from_ptr (struct value *array_ptr, struct type *type,
>    if (stride == 0)
>      stride = TYPE_LENGTH (TYPE_TARGET_TYPE (type0));
>  
> -  base = value_as_address (array_ptr) + (low_pos - base_low_pos) * stride;
> +  base = value_as_address (array_ptr) + (*low_pos - *base_low_pos) * stride;
>    return value_at_lazy (slice_type, base);
>  }
>  
> @@ -2848,10 +2850,13 @@ ada_value_slice (struct value *array, int low, int high)
>  			      (NULL, TYPE_TARGET_TYPE (type), index_type,
>  			       type->dyn_prop (DYN_PROP_BYTE_STRIDE),
>  			       TYPE_FIELD_BITSIZE (type, 0));
> -  LONGEST low_pos, high_pos;
> +  gdb::optional<LONGEST> low_pos, high_pos;
> +
>  
> -  if (!discrete_position (base_index_type, low, &low_pos)
> -      || !discrete_position (base_index_type, high, &high_pos))
> +  low_pos = discrete_position (base_index_type, low);
> +  high_pos = discrete_position (base_index_type, high);
> +
> +  if (!low_pos.has_value () || !high_pos.has_value ())
>      {
>        warning (_("unable to get positions in slice, use bounds instead"));
>        low_pos = low;
> @@ -2859,7 +2864,7 @@ ada_value_slice (struct value *array, int low, int high)
>      }
>  
>    return value_cast (slice_type,
> -		     value_slice (array, low, high_pos - low_pos + 1));
> +		     value_slice (array, low, *high_pos - *low_pos + 1));
>  }
>  
>  /* If type is a record type in the form of a standard GNAT array
> @@ -8929,15 +8934,15 @@ pos_atr (struct value *arg)
>  {
>    struct value *val = coerce_ref (arg);
>    struct type *type = value_type (val);
> -  LONGEST result;
>  
>    if (!discrete_type_p (type))
>      error (_("'POS only defined on discrete types"));
>  
> -  if (!discrete_position (type, value_as_long (val), &result))
> +  gdb::optional<LONGEST> result = discrete_position (type, value_as_long (val));
> +  if (!result.has_value ())
>      error (_("enumeration value is invalid: can't find 'POS"));
>  
> -  return result;
> +  return *result;
>  }
>  
>  static struct value *
> diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
> index e6f70bbe2d3..09f33c21e28 100644
> --- a/gdb/gdbtypes.c
> +++ b/gdb/gdbtypes.c
> @@ -1062,9 +1062,21 @@ get_discrete_bounds (struct type *type, LONGEST *lowp, LONGEST *highp)
>  
>        if (TYPE_TARGET_TYPE (type)->code () == TYPE_CODE_ENUM)
>  	{
> -	  if (!discrete_position (TYPE_TARGET_TYPE (type), *lowp, lowp)
> -	      || ! discrete_position (TYPE_TARGET_TYPE (type), *highp, highp))
> +	  gdb::optional<LONGEST> low_pos
> +	    = discrete_position (TYPE_TARGET_TYPE (type), *lowp);
> +
> +	  if (!low_pos.has_value ())
> +	    return 0;
> +
> +	  *lowp = *low_pos;
> +
> +	  gdb::optional<LONGEST> high_pos
> +	    = discrete_position (TYPE_TARGET_TYPE (type), *highp);
> +
> +	  if (!high_pos.has_value ())
>  	    return 0;
> +
> +	  *highp = *high_pos;
>  	}
>        return 1;
>      case TYPE_CODE_ENUM:
> @@ -1160,8 +1172,8 @@ get_array_bounds (struct type *type, LONGEST *low_bound, LONGEST *high_bound)
>     in which case the value of POS is unmodified.
>  */
>  
> -int
> -discrete_position (struct type *type, LONGEST val, LONGEST *pos)
> +gdb::optional<LONGEST>
> +discrete_position (struct type *type, LONGEST val)
>  {
>    if (type->code () == TYPE_CODE_RANGE)
>      type = TYPE_TARGET_TYPE (type);
> @@ -1173,19 +1185,14 @@ discrete_position (struct type *type, LONGEST val, LONGEST *pos)
>        for (i = 0; i < type->num_fields (); i += 1)
>  	{
>  	  if (val == TYPE_FIELD_ENUMVAL (type, i))
> -	    {
> -	      *pos = i;
> -	      return 1;
> -	    }
> +	    return i;
>  	}
> +
>        /* Invalid enumeration value.  */
> -      return 0;
> +      return {};
>      }
>    else
> -    {
> -      *pos = val;
> -      return 1;
> -    }
> +    return val;
>  }
>  
>  /* If the array TYPE has static bounds calculate and update its
> diff --git a/gdb/gdbtypes.h b/gdb/gdbtypes.h
> index 2b6f599f4c7..59ff6fc6ce3 100644
> --- a/gdb/gdbtypes.h
> +++ b/gdb/gdbtypes.h
> @@ -46,6 +46,7 @@
>  
>  #include "hashtab.h"
>  #include "gdbsupport/array-view.h"
> +#include "gdbsupport/gdb_optional.h"
>  #include "gdbsupport/offset-type.h"
>  #include "gdbsupport/enum-flags.h"
>  #include "gdbsupport/underlying.h"
> @@ -2445,7 +2446,8 @@ extern int get_discrete_bounds (struct type *, LONGEST *, LONGEST *);
>  extern bool get_array_bounds (struct type *type, LONGEST *low_bound,
>  			      LONGEST *high_bound);
>  
> -extern int discrete_position (struct type *type, LONGEST val, LONGEST *pos);
> +extern gdb::optional<LONGEST> discrete_position (struct type *type,
> +						 LONGEST val);
>  
>  extern int class_types_same_p (const struct type *, const struct type *);
>  
> -- 
> 2.29.2

-- 
Joel