Re: [PATCH v2 1/4] gdb: Add typesafe getter/setter for cmd_list_element.var

[Simon Marchi via Gdb-patches <gdb-patches@sourceware.org>]

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<bool> () const
> 	{
> 	  gdb_assert (this->m_var_type == var_boolean);
> 	  gdb_assert (this->m_var != nullptr);
> 	  return *static_cast<bool *> (this->m_var);
> 	}
> 	void set<bool> (bool var)
> 	{
> 	  gdb_assert (this->m_var_type == var_boolean);
> 	  gdb_assert (this->m_var != nullptr);
> 	  *static_cast<bool *> (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<typename T>
> +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<bool> (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<enum auto_boolean> (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<unsigned int> (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<int> (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<char *> (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<const char *> (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<typename T>
> +  T const *
> +  get_p () const
> +  {
> +    gdb_assert (var_type_uses<T> (this->m_var_type));
> +    gdb_assert (!this->empty ());
> +
> +    return static_cast<T const *> (this->m_var);
> +  }
> +
> +  /* Return the current value.
> +
> +     See get_p for discussion on the return type and template parameters.  */
> +  template<typename T>
> +  T get () const
> +  {
> +    return *get_p<T> ();
> +  }
> +
> +  /* 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<typename T>
> +  void set (T v)
> +  {
> +    /* Check that the current instance is of one of the supported types for
> +       this instantiation.  */
> +    gdb_assert (var_type_uses<T> (this->m_var_type));
> +    gdb_assert (!this->empty ());
> +
> +    *static_cast<T *> (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 <gdb:exception> 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