From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from lndn.lancelotsix.com (lndn.lancelotsix.com [51.195.220.111]) by sourceware.org (Postfix) with ESMTPS id 1D5B638515F3 for ; Mon, 19 Jul 2021 20:58:32 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 1D5B638515F3 Received: from Plymouth (unknown [IPv6:2a02:390:9086:0:dd36:20ad:c06b:f72]) by lndn.lancelotsix.com (Postfix) with ESMTPSA id 7E5D981D69; Mon, 19 Jul 2021 20:58:30 +0000 (UTC) Date: Mon, 19 Jul 2021 21:58:27 +0100 From: Lancelot SIX To: Simon Marchi Cc: gdb-patches@sourceware.org Subject: Re: [PATCH 15/16] gdb: make cmd_list_element var an optional union Message-ID: <20210719205804.cy4etmcgntu2bbdh@Plymouth> References: <20210714045520.1623120-1-simon.marchi@polymtl.ca> <20210714045520.1623120-16-simon.marchi@polymtl.ca> <20210718154429.27arnsgirctcjett@ubuntu.lan> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.5.11 (lndn.lancelotsix.com [0.0.0.0]); Mon, 19 Jul 2021 20:58:30 +0000 (UTC) X-Spam-Status: No, score=-4.8 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) 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: Mon, 19 Jul 2021 20:58:34 -0000 On Mon, Jul 19, 2021 at 10:19:21AM -0400, Simon Marchi wrote: > > + setting_variable v { .bool_var = ¶m_value->boolval }; > > + return pascm_param_value (type, v, arg_pos, func_name); > > I thought about doing something like this, my only itch is that > declaration of `setting_variable v`. It is confusing to see it just > setting the bool_var field, regardless of the actual type. I understand > it works because the pointer value is the same in any case, but to be > pedantic it should be something like: > > setting_variable v; > switch (type) > { > case var_boolean: > v.bool_var = ¶m_value->boolval; > break; > case var_auto_boolean: > v.auto_boolean_var = ¶m_value->autoboolval; > break; > // and so on > } > > It's a bit more verbose, but it's written only once, and it's not like > new var types are added every week. > > I would be fine changing my patch to use your approach with that change, > if you are. > > Simon When sending my comment, I was wondering about suggesting just that. It is definitely more verbose, but I would not mind that. I tend to prefer verbose code over code duplication. It is too easy to update only one of the two implementations and have behaviors diverge. Just out of curiosity, I have checked with Compiler Explorer[1], and starting with -01, the entire switch goes away (when using __builtin_unreachable, without it the switch is dropped but a check is maintained). This is surely not a hot code path so this does not changes much, but it always nice to know the compiler can do the clever thing. Lancelot. [1] https://godbolt.org/z/GdqGno6cq