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 5E623386480C for ; Tue, 10 Aug 2021 01:30:58 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 5E623386480C 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 17A1TnRR010647 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Mon, 9 Aug 2021 21:29:54 -0400 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp.polymtl.ca 17A1TnRR010647 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 D91F11E79C; Mon, 9 Aug 2021 21:29:48 -0400 (EDT) Subject: Re: [PATCH v2 1/4] gdb: Add typesafe getter/setter for cmd_list_element.var To: Lancelot SIX , gdb-patches@sourceware.org References: <20210808192302.3768766-1-lsix@lancelotsix.com> <20210808192302.3768766-2-lsix@lancelotsix.com> From: Simon Marchi Message-ID: Date: Mon, 9 Aug 2021 21:29:48 -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: <20210808192302.3768766-2-lsix@lancelotsix.com> 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 Tue, 10 Aug 2021 01:29:49 +0000 X-Spam-Status: No, score=-10.0 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, 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: Tue, 10 Aug 2021 01:31:08 -0000 On 2021-08-08 3:22 p.m., Lancelot SIX via Gdb-patches wrote: > cmd_list_element can contain a pointer to data that can be set and / or > shown. This is achieved with a void* that points to the data that can > be accessed, while the var_type (of type enum var_types) tells how to > interpret the data pointed to. > > With this pattern, the user of the cmd_list_element needs to know what > the storage type associated with a given VAR_TYPES tag, and needs to do > the proper casting. No automatic check at compile or run time enforces > that the adequate cast is done. > > This looks something like: > > if (c->var_type == var_zuinteger) > unsigned int v = *(unsigned int*)v->var_type; > > With this approach, it is easy to make an error. > > This patch aims at addressing the same problem as > https://sourceware.org/pipermail/gdb-patches/2021-July/180920.html but > uses a different approach. > > This patch proposes to add an abstraction around the var_types and void* > pointer pair. The abstraction is meant to prevent the user from having > to handle the cast. This is achieved by introducing the setting > struct, and a set of templated get / set member functions. The template > parameter is the type of the variable that holds the referred variable. > > For example, instantiating the member functions with bool will yield > something similar to: > > bool get () const > { > gdb_assert (this->m_var_type == var_boolean); > gdb_assert (this->m_var != nullptr); > return *static_cast (this->m_var); > } > void set (bool var) > { > gdb_assert (this->m_var_type == var_boolean); > gdb_assert (this->m_var != nullptr); > *static_cast (this->m_var) = var; > } > > With such abstractions available, the var_type and var members of the > cmd_list_element are replaced with a setting instance. > > No user visible change is expected after this patch. > > Tested on GNU/Linux x86_64, with no regression noticed. Thanks, this is OK, with the following nits fixed. > diff --git a/gdb/command.h b/gdb/command.h > index baf34401a07..644812c4d46 100644 > --- a/gdb/command.h > +++ b/gdb/command.h > @@ -125,6 +125,164 @@ typedef enum var_types > } > var_types; > > +/* Return true if a setting of type VAR_TYPE is backed with type T. > + > + This function is left without definition intentionally. This template Double space. > + is specialized for all valid types that are used to back var_types. > + Therefore if one tries to instantiate this un-specialized template it > + means the T parameter is not a type used to back a var_type and it is most > + likely a programming error. */ > +template > +inline bool var_type_uses (var_types t); > + > +/* Return true if a setting of type T is backed by a bool variable. */ > +template<> > +inline bool var_type_uses (var_types t) > +{ > + return t == var_boolean; > +}; > + > +/* Return true if a setting of type T is backed by a auto_boolean variable. > +*/ > +template<> > +inline bool var_type_uses (var_types t) > +{ > + return t == var_auto_boolean; > +} > + > +/* Return true if a setting of type T is backed by an unsigned int variable. > +*/ > +template<> > +inline bool var_type_uses (var_types t) > +{ > + return (t == var_uinteger || t == var_zinteger || t == var_zuinteger); > +} > + > +/* Return true if a setting of type T is backed by an int variable. */ > +template<> > +inline bool var_type_uses (var_types t) > +{ > + return (t == var_integer || t == var_zinteger > + || t == var_zuinteger_unlimited); > +} > + > +/* Return true if a setting of type T is backed by a char * variable. */ > +template<> > +inline bool var_type_uses (var_types t) > +{ > + return (t == var_string || t == var_string_noescape > + || t == var_optional_filename || t == var_filename); > +} > + > +/* Return true if a setting of type T is backed by a const char * variable. > +*/ > +template<> > +inline bool var_type_uses (var_types t) > +{ > + return t == var_enum; > +} > + > +/* Abstraction that contains access to data that can be set or shown. > + > + The underlying data can be of any VAR_TYPES type. */ > +struct base_setting > +{ > + /* Access the type of the current var. */ > + var_types type () const > + { > + return m_var_type; > + } > + > + /* Return the current value (by pointer). > + > + The template parameter T is the type of the variable used to store the > + setting. > + > + The returned value cannot be NULL (this is checked at runtime). */ > + template > + T const * > + get_p () const > + { > + gdb_assert (var_type_uses (this->m_var_type)); > + gdb_assert (!this->empty ()); > + > + return static_cast (this->m_var); > + } > + > + /* Return the current value. > + > + See get_p for discussion on the return type and template parameters. */ > + template > + T get () const > + { > + return *get_p (); > + } > + > + /* Sets the value V to the underlying buffer. > + > + The template parameter T indicates the type of the variable used to store > + the setting. > + > + The var_type of the setting must match T. */ > + template > + void set (T v) > + { > + /* Check that the current instance is of one of the supported types for > + this instantiation. */ > + gdb_assert (var_type_uses (this->m_var_type)); > + gdb_assert (!this->empty ()); > + > + *static_cast (this->m_var) = v; > + } > + > + /* A setting is valid (can be evaluated to true) if it contains a valid > + reference to a memory buffer. */ > + explicit operator bool () const > + { > + return !this->empty (); > + } > + > +protected: > + /* The type of the variable M_VAR is pointing to. If M_VAR is nullptr, > + M_VAR_TYPE is ignored. */ > + var_types m_var_type { var_boolean }; > + > + /* Pointer to the enclosed variable. The type of the variable is encoded > + in M_VAR_TYPE. Can be nullptr. */ > + void *m_var { nullptr }; > + > + /* Indicates if the current instance has a underlying buffer. */ > + bool empty () const > + { > + return m_var == nullptr; > + } > + > +}; > + > + > +/* A base_setting with additional methods to set the underlying buffer and Remove extra empty line above. > @@ -566,14 +577,13 @@ pascm_param_type_name (enum var_types param_type) > If TYPE is not supported, then a object is returned. */ > > static SCM > -pascm_param_value (enum var_types type, void *var, > - int arg_pos, const char *func_name) > +pascm_param_value (base_setting const &var, int arg_pos, const char *func_name) The comment "If TYPE is not supported..." should be updated. Simon