From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from rock.gnat.com (rock.gnat.com [205.232.38.15]) by sourceware.org (Postfix) with ESMTP id 4DEDB388A43C for ; Sun, 6 Dec 2020 05:25:19 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 4DEDB388A43C Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=adacore.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=brobecker@adacore.com Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 2A132116C82; Sun, 6 Dec 2020 00:25:19 -0500 (EST) X-Virus-Scanned: Debian amavisd-new at gnat.com Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id EhafqGx49xAb; Sun, 6 Dec 2020 00:25:19 -0500 (EST) Received: from float.home (localhost.localdomain [127.0.0.1]) (using TLSv1.2 with cipher ADH-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by rock.gnat.com (Postfix) with ESMTPS id C82BD116C40; Sun, 6 Dec 2020 00:25:18 -0500 (EST) Received: by float.home (Postfix, from userid 1000) id E08B8A1880; Sun, 6 Dec 2020 09:25:13 +0400 (+04) Date: Sun, 6 Dec 2020 09:25:13 +0400 From: Joel Brobecker To: Simon Marchi via Gdb-patches Cc: Simon Marchi Subject: Re: [PATCH 1/4] gdb: make discrete_position return optional Message-ID: <20201206052513.GB321750@adacore.com> References: <20201123162120.3778679-1-simon.marchi@efficios.com> <20201123162120.3778679-2-simon.marchi@efficios.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20201123162120.3778679-2-simon.marchi@efficios.com> X-Spam-Status: No, score=-9.8 required=5.0 tests=BAYES_00, GIT_PATCH_0, KAM_DMARC_STATUS, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 06 Dec 2020 05:25:21 -0000 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 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 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 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 low_pos > + = discrete_position (TYPE_TARGET_TYPE (type), *lowp); > + > + if (!low_pos.has_value ()) > + return 0; > + > + *lowp = *low_pos; > + > + gdb::optional 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 > +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 discrete_position (struct type *type, > + LONGEST val); > > extern int class_types_same_p (const struct type *, const struct type *); > > -- > 2.29.2 -- Joel