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 24309385801D for ; Wed, 14 Jul 2021 19:23:51 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 24309385801D 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 16EJMgoi004017 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Wed, 14 Jul 2021 15:22:47 -0400 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp.polymtl.ca 16EJMgoi004017 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) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id EE8C71E4A3; Wed, 14 Jul 2021 15:22:41 -0400 (EDT) Subject: Re: [PATCH 15/16] gdb: make cmd_list_element var an optional union To: Lancelot SIX , 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> From: Simon Marchi Message-ID: Date: Wed, 14 Jul 2021 15:22:41 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.11.0 MIME-Version: 1.0 In-Reply-To: <20210714171238.vzccwpurh2izbkps@ubuntu.lan> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit X-Poly-FromMTA: (simark.ca [158.69.221.121]) at Wed, 14 Jul 2021 19:22:42 +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: Wed, 14 Jul 2021 19:23:54 -0000 On 2021-07-14 1:12 p.m., Lancelot SIX wrote: >> I happen to have been working on a different approach to the same >> problem. This was triggered when I was reading one of your previous >> patches where the surrounding codes had a lot of C style casts all >> around. My approach is arguably too complicated for the task, but it >> is meant to address (at least partly) the last point you discuss. Hi Lancelot, Thanks for taking a look, good to know that somebody else felt the need to modernize this code. >> Here is the gist of my approach: I add templated getter / setter to the >> var member, the template parameter being the var_type you expect. The >> usage looks like this, if you want to access the value of a >> cmd_list_element of var_types var_zuinteger: >> >> cmd_list_element *el; >> unsigned int val = el->get_var (); // This type checks. >> >> The implementation for that requires a bit of boiler plate, but is not >> too complex to follow. I add a helper struct I use to describe the >> storage type used or each var_types: >> >> template struct var_types_storage { }; >> template<> struct var_types_storage { using t = int; } ; >> template<> struct var_types_storage { using t = unsigned int; }; >> template<> struct var_types_storage { using t = char *; }; >> template<> struct var_types_storage { using t = const char *; }; >> // And so on for all the possible var type >> >> With that available, the getter / setter is quite easy to write: >> >> template >> typename var_types_storage::t get_var () const >> { >> gdb_assert (T == this->var_type); >> return *static_cast::t *> (this->var); >> } >> >> template >> void set_var (typename var_types_storage::t v) >> { >> gdb_assert (T == this->var_type); >> *static_cast::t *> (this->var> = v; >> } >> >> template >> void set_var_p (typename var_types_storage::t v) >> { >> this->var_type = T; >> this->var = static_cast v; >> } >> >> add_set_or_show_cmd can also be updated in a similar way: >> >> template static struct cmd_list_element * >> add_set_or_show_cmd (const char *name, >> enum cmd_types type, >> enum command_class theclass, >> typename var_types_storage::t *var, >> const char *doc, >> struct cmd_list_element **list) >> { >> struct cmd_list_element *c = add_cmd (name, theclass, doc, list); >> c->set_var_p (var); >> // rest is unchanged >> return c; >> } >> >> // similar stuff for add_setshow_cmd_full >> >> Obviously there are a few differences with your approach. >> >> First, with my approach, it is possible to add the gdb_assert to check >> that when one accesses a value assuming it is of a given type, it >> actually is. However, there are few limitations with that, most >> notably in gdb/cli/cli-setshow.c, where some processing is common >> between cases (var_uinteger and var_zuinteger have quite a lot in >> common for example). At some point, the caller wants to call >> set_var, but obviously the >> assertion will fail half of the time. To go around this, I also use less >> type-safe setters / getters: >> >> template >> const T get_var_as() { return *static_cast (this->var); } >> template >> void set_var_as (T v) { *static_cast (this->var) = v; } >> >> Second, with the templated approach the caller does not need to know >> which member of the union he should access for a given var_types, the >> compiler will tel him if an incompatible types are used on the call >> site. >> >> On the other hand, my approach might be more difficult to follow / more >> surprising for someone not familiar with this kind of use of template >> specialization. It also requires a bit more boiler plate code (i.e. the >> var_types_storage struct). >> >> I still rely on type punning and a underlying void * pointer, which is >> not ideal. But at least, this becomes a detail of the implementation >> and can be ignored by the caller with all the casting done in just one >> place. The void * pointer should (in my approach) eventually be turned >> into a private member, which I have not done yet. >> >> Anyway, your approach is quite an improvement compared to the current >> type casting all over the place. I guess that we could, if you want, >> have a solution which which is a hybrid of both (i.e. a union instead of >> a void*, and templated access functions). >> >> Obviously I am a bit biased toward my approach. Any thoughts? I’ll >> pause my work on my patch for the moment. >> >> I still need to give a proper read to your patch series, I just skimmed >> through to compare our approaches for the moment. >> >> Best, >> Lancelot. > > Actually, I have been playing around with it a bit more today, and with > extra boiler plate, I can remove the get_var_as accessor, and have a > more all-rounded get_var. > > It becomes possible to write something like: > > // No matter if we are looking at a var_uinteger or a > // var_zuinteger, the underlying type we are interested in is > // a unsigned int. > unsigned int *foo = c->get_var (); > > In this case, get_var will even figure the common ground for all > possibilities the user asks for. The intended use case is when printing > the value of a string (ish) variable, and we want to cover multiple > scenarios including var_enum and var_string, we can write something > like: > > // c is a const char *, but would be char * if var_enum was not > // in the list. > auto c = v.get_var (); > > The code could look like something like this (I am sure it is possible > to have a simpler solution to create a similar result): > > template > struct var_types_storage_helper; > > template > struct var_types_storage_helper > { > static constexpr bool all_same = true; > using type = typename var_types_storage::t; > static constexpr bool covers_var_type (var_types t) > { > return t == T; > } > }; > > template > struct var_types_storage_helper > { > static constexpr bool all_same = std::is_same::t, typename var_types_storage::t>::value; > using type = typename var_types_storage::t; > static constexpr bool covers_var_type (var_types t) > { > return var_types_storage_helper::covers_var_type (t) || var_types_storage_helper::covers_var_type (t); > } > }; > > template > struct var_types_storage_helper > { > static constexpr bool all_same = var_types_storage_helper::all_same && var_types_storage_helper::all_same; > using type = typename var_types_storage::t; > static constexpr bool covers_var_type (var_types t) > { > return var_types_storage_helper (t) || var_types_storage_helper::covers_var_type (t); > } > }; > > and the accessors become: > > template > typename std::common_type::t...>::type get_var() const > { > assert (var_types_storage_helper::covers_var_type (this->var_type)); > return *static_cast::t...>::type *>(this->var); > } > > template > void set_var(typename var_types_storage_helper::type v) > { > assert (var_types_storage_helper::covers_var_type (this->var_type)); > static_assert (var_types_storage_helper::all_same); > *static_cast::type *> (this->var) = v; > } > > template > void set_var_p (typename var_types_storage::t *v) > { > this->var_type = T; > this->var = static_cast (v); > } > > I guess improves the type safety side of things, but the > implementation definitely gets more complicated. I am not sure the > added checks come at a reasonable price. > > This should all be valid c++11. > > Any thoughts? 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