From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca (simark.ca [158.69.221.121]) by sourceware.org (Postfix) with ESMTPS id 86D113973034 for ; Sat, 19 Sep 2020 13:50:42 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 86D113973034 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=simark.ca Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=simark@simark.ca Received: from [10.0.0.11] (173-246-6-90.qc.cable.ebox.net [173.246.6.90]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id C96AA1E599; Sat, 19 Sep 2020 09:50:41 -0400 (EDT) Subject: Re: [PATCHv3 1/2] gdb: Convert enum range_type to a bit field enum To: Andrew Burgess , gdb-patches@sourceware.org References: From: Simon Marchi Message-ID: <3c557be0-e07c-2894-322c-65559f695b61@simark.ca> Date: Sat, 19 Sep 2020 09:50:41 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.12.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: fr Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-9.8 required=5.0 tests=BAYES_00, GIT_PATCH_0, KAM_DMARC_STATUS, NICE_REPLY_A, SPF_HELO_PASS, 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: Sat, 19 Sep 2020 13:50:43 -0000 On 2020-09-19 5:48 a.m., Andrew Burgess wrote: > The expression range_type enum represents the following ideas: > > - Lower bound is set to default, > - Upper bound is set to default, > - Upper bound is exclusive. > > There are currently 6 entries in the enum to represent the combination > of all those ideas. > > In a future commit I'd like to add stride information to the range, > this could in theory appear with any of the existing enum entries, so > this would take us to 12 enum entries. > > This feels like its getting a little out of hand, so in this commit I > switch the range_type enum over to being a flags style enum. There's > one entry to represent no flags being set, then 3 flags to represent > the 3 ideas above. Adding stride information will require adding only > one more enum flag. > > I've then gone through and updated the code to handle this change. > > There should be no user visible changes after this commit. I think it's a good idea. I noted a few comments below. > diff --git a/gdb/expression.h b/gdb/expression.h > index 5af10f05db1..9dc598984e0 100644 > --- a/gdb/expression.h > +++ b/gdb/expression.h > @@ -187,20 +187,20 @@ extern void dump_prefix_expression (struct expression *, struct ui_file *); > > enum range_type > { > - /* Neither the low nor the high bound was given -- so this refers to > - the entire available range. */ > - BOTH_BOUND_DEFAULT, > + /* This is a standard range. Both the lower and upper bounds are > + defined, and the bounds are inclusive. */ > + RANGE_STANDARD = 0, > + > /* The low bound was not given and the high bound is inclusive. */ > - LOW_BOUND_DEFAULT, > + RANGE_LOW_BOUND_DEFAULT = 1 << 0, > + > /* The high bound was not given and the low bound in inclusive. */ > - HIGH_BOUND_DEFAULT, > - /* Both bounds were given and both are inclusive. */ > - NONE_BOUND_DEFAULT, > - /* The low bound was not given and the high bound is exclusive. */ > - NONE_BOUND_DEFAULT_EXCLUSIVE, > - /* Both bounds were given. The low bound is inclusive and the high > - bound is exclusive. */ > - LOW_BOUND_DEFAULT_EXCLUSIVE, > + RANGE_HIGH_BOUND_DEFAULT = 1 << 1, I don't think the comments on RANGE_LOW_BOUND_DEFAULT and RANGE_HIGH_BOUND_DEFAULT should mention that the other bound is inclusive anymore, since that's controlled by another flag. > +DEF_ENUM_FLAGS_TYPE (enum range_type, range_types); I think the type name is confusing. I thought at first it was related to a TYPE_CODE_RANGE `struct type`. What about renaming to "enum range_flag" and the enum flags type to "range_flags"? > diff --git a/gdb/rust-exp.y b/gdb/rust-exp.y > index db888098c4a..5111de9cf9c 100644 > --- a/gdb/rust-exp.y > +++ b/gdb/rust-exp.y > @@ -2492,24 +2492,29 @@ rust_parser::convert_ast_to_expression (const struct rust_op *operation, > > case OP_RANGE: > { > - enum range_type kind = BOTH_BOUND_DEFAULT; > + enum range_type kind = (RANGE_HIGH_BOUND_DEFAULT > + | RANGE_LOW_BOUND_DEFAULT); > > if (operation->left.op != NULL) > { > convert_ast_to_expression (operation->left.op, top); > - kind = HIGH_BOUND_DEFAULT; > + kind = RANGE_HIGH_BOUND_DEFAULT; What I understand from this code is: if the low bound (left) is provided to the range operator, we want to clear the RANGE_LOW_BOUND_DEFAULT bit. So I think it would be more natural to write in the typical form to clear bits: kind &= ~RANGE_LOW_BOUND_DEFAULT; I think it expresses the intent better. You'll need to make the enum type explicitly unsigned to be able to use the ~ operator. Simon