From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id vWvTNSPXEWHDUwAAWB0awg (envelope-from ) for ; Mon, 09 Aug 2021 21:32:19 -0400 Received: by simark.ca (Postfix, from userid 112) id CC5A71EDF7; Mon, 9 Aug 2021 21:32:19 -0400 (EDT) X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-0.7 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,MAILING_LIST_MULTI,RDNS_DYNAMIC,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.2 Received: from sourceware.org (ip-8-43-85-97.sourceware.org [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id CB40A1E79C for ; Mon, 9 Aug 2021 21:32:18 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id B2E9E384A07D for ; Tue, 10 Aug 2021 01:32:17 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org B2E9E384A07D DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1628559137; bh=ocwe3Z0QdmqhRI5ou6iUFbQFifLB6GzdmgBsyAh4xMs=; h=Subject:To:References:Date:In-Reply-To:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To: From; b=HkZVgrozneIDNfDqMYxtkZ2/W9Ak+NzHWW6mUJl2yaMl4EYB5+EsQxT0vDveld7Ph +6kKXSGbpUIS8ZxoJq+xhWLraWAhychgdqzTRA6Xhl5BUVVeiQx2lTFpMRETUwrAQq /IE+G6OvugN6K1gcjT6df+rM7PZftSihLlnzjCA4= 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> 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-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: , From: Simon Marchi via Gdb-patches Reply-To: Simon Marchi Errors-To: gdb-patches-bounces+public-inbox=simark.ca@sourceware.org Sender: "Gdb-patches" 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