From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp.polymtl.ca (smtp.polymtl.ca [132.207.4.11]) by sourceware.org (Postfix) with ESMTPS id 31461384843A for ; Mon, 19 Jul 2021 14:33:17 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 31461384843A Received: from simark.ca (simark.ca [158.69.221.121]) (authenticated bits=0) by smtp.polymtl.ca (8.14.7/8.14.7) with ESMTP id 16JEWAn3020677 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Mon, 19 Jul 2021 10:32:15 -0400 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp.polymtl.ca 16JEWAn3020677 Received: from [10.0.0.11] (192-222-157-6.qc.cable.ebox.net [192.222.157.6]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 1F2E11E4A3; Mon, 19 Jul 2021 10:32:10 -0400 (EDT) Subject: Re: [PATCH 15/16] gdb: make cmd_list_element var an optional union To: Lancelot SIX Cc: gdb-patches@sourceware.org References: <20210714045520.1623120-1-simon.marchi@polymtl.ca> <20210714045520.1623120-16-simon.marchi@polymtl.ca> <20210714120851.3pfew5pgcdp6ezn6@ubuntu.lan> <20210714171238.vzccwpurh2izbkps@ubuntu.lan> <20210714232112.wsn7pits6uuz3nf5@ubuntu.lan> From: Simon Marchi Message-ID: Date: Mon, 19 Jul 2021 10:32:09 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.12.0 MIME-Version: 1.0 In-Reply-To: <20210714232112.wsn7pits6uuz3nf5@ubuntu.lan> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Poly-FromMTA: (simark.ca [158.69.221.121]) at Mon, 19 Jul 2021 14:32:10 +0000 X-Spam-Status: No, score=-4.0 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, NICE_REPLY_A, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_PASS, 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 14:33:18 -0000 On 2021-07-14 7:22 p.m., Lancelot SIX wrote: > On Wed, Jul 14, 2021 at 03:22:41PM -0400, Simon Marchi wrote: >> >> I am certainly for this kind of consistency checks, making sure we >> access the right data the right way. This is what I did with struct >> dynamic_prop, for example. >> >> I was thinking of adding accessors too, eventually. I am not very good >> with advanced C++ templates though, so what I had in mind was more >> something like the obvious: >> >> struct cmd_list_element >> { >> int *var_as_int () // or int & >> { >> gdb_assert (m_var_type == a >> || m_var_type == b); >> return m_var.int_var; >> } >> >> unsigned int *var_as_unsigned_int () // or unsigned int & >> { >> gdb_assert (m_var_type == c >> || m_var_type == c); >> return m_var.unsigned_int_var; >> } >> >> // And so on. >> }; >> >> Your last proposition is a bit better in the sense that you ask "give me >> the variable location for a var_zuinteger". With mine, you still have >> to know that a var_zuinteger uses the unsigned int member. But at >> least, there's a gdb_assert to catch mistakes at runtime. >> >> Whether it's worth the extra complexity... I can't tell, I would have to >> see the complete patch. I would also have to see how that fits with the >> upcoming fix I have for bug 28085. In the mean time, does it make your >> life more difficult if we merge my patch? I would guess not, since >> you'll need to modify all the call sites (places that access setting >> variables) anyway. You can then decide to keep the union or not in your >> patch. >> >> Simon > > I have not yet finished updating all callsites, but here is a > (hopefully) working patch if you want to see what it looks like. > > I have not had a look at 28085 yet, so I do not yet if my approach gets > in the way of fixing it. For the moment, I do not have something > similar to the gdb_optional you introduce, but it should not be too > difficult to have something similar. > > Anyway, to not hold back on my account. I have not read your series > carefully yet, but it seems a real improvement from the current > situation. If it helps fixing a bug, it is even better. I can rework > my patch to work on top of this one (at least if what it brings is > considered worthwhile). > > Lancelot. I took a quick look, and that seems reasonable to me. I don't think it will conflict (in design) with my upcoming patch. My upcoming patch makes it so that some settings don't use the storage that is in cmd_list_element at all, but instead provide some getter/setter callback. This is to fix display of settings where the "source of truth" is not cmd_list_element::var. With my patch, the code at various places becomes, conceptually: if (cmd_list_element::var is set) use cmd_list_element::var else use the getter/setter I think it would be nice to make the first case (using cmd_list_element::var for storage) just a specific case of using the getter/setter. That is, when we create a setting that uses cmd_list_element::var for storage, we install a getter and a setter than get and set the value in cmd_list_element::var. But I haven't gotten there yet. You'll probably have some good ideas for achieving this :). But before posting that patch, I'd like to decide how the current patch series will end up, because that will very much affect how the next patch will look like. Simon