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

Joel Brobecker
2020-12-06T05:25:24Z

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