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

[Simon Marchi <simon.marchi@polymtl.ca>]

Hi Lancelot,

This looks great, thanks.  It is adequately commented, making easy
enough to grok.  I put some minor comments below.

On 2021-07-28 3:50 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.
> 
> 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 param_ref

Unfortunately, both the "setting" and "parameter" words are used.  The
later is present in the Python and Guile interfaces, so it can't easily
be removed.  But at least, in the sources, I'd like if we could use
"setting" consistently.

> struct, and a set of templated get / set member functions.  The template
> parameter is the VAR_TYPES we want to the access the data for.  The
> template ensures the return type of the get method and the parameter
> type of the get  method matches the data of the underlying data.
> 
> For example, instantiating the member functions with var_boolean will
> yield something similar to:
> 
> 	bool get<var_boolean> () const
> 	{
> 	  gdb_assert (this->m_var_type == var_boolean);
> 	  gdb_assert (this->m_var != nullptr);
> 	  return *static_cast<bool *> (this->m_var);
> 	}
> 	void set<var_boolean> (bool var)
> 	{
> 	  gdb_assert (this->m_var_type == var_boolean);
> 	  gdb_assert (this->m_var != nullptr);
> 	  *static_cast<bool *> (this->m_var) = var;
> 	}
> 
> With those new methods, the caller does not need to know the underlying
> storage type nor does he need to perform the cast manually.  This combined
> with the added dynamic checks (gdb_assert) makes this approach way safer
> in my opinion.
> 
> Given that the processing of some options is common between VAR_TYPES,
> the templates can be instantiated with more than one VAR_TYPES
> arguments.  In such situation, all the template parameters need to
> describe options that share the same underlying storage type.  This

It sounds like there are some missing words in this last sentence.

> diff --git a/gdb/command.h b/gdb/command.h
> index baf34401a07..b6481c066ce 100644
> --- a/gdb/command.h
> +++ b/gdb/command.h
> @@ -125,6 +125,272 @@ typedef enum var_types
>    }
>  var_types;
>  
> +/* Holds implementation details for the setting_ref wrapper.  */
> +namespace detail
> +{
> +  /* Helper classes used to associate a storage type for each possible
> +     var_type. */
> +  template<var_types T>
> +  struct var_types_storage;
> +
> +  template<>
> +  struct var_types_storage<var_boolean>
> +  {
> +    using type = bool;
> +  };
> +
> +  template<>
> +  struct var_types_storage<var_auto_boolean>
> +  {
> +    using type = auto_boolean;
> +  };
> +
> +  template<>
> +  struct var_types_storage<var_uinteger>
> +  {
> +    using type = unsigned int;
> +  };
> +
> +  template<>
> +  struct var_types_storage<var_integer>
> +  {
> +    using type = int;
> +  };
> +
> +  template<>
> +  struct var_types_storage<var_string>
> +  {
> +    using type = char *;
> +  };
> +
> +  template<>
> +  struct var_types_storage<var_string_noescape>
> +  {
> +    using type = char *;
> +  };
> +
> +  template<>
> +  struct var_types_storage<var_optional_filename>
> +  {
> +    using type = char *;
> +  };
> +
> +  template<>
> +  struct var_types_storage<var_filename>
> +  {
> +    using type = char *;
> +  };
> +
> +  template<>
> +  struct var_types_storage<var_zinteger>
> +  {
> +    using type = int;
> +  };
> +
> +  template<>
> +  struct var_types_storage<var_zuinteger>
> +  {
> +    using type = unsigned int;
> +  };
> +
> +  template<>
> +  struct var_types_storage<var_zuinteger_unlimited>
> +  {
> +    using type = int;
> +  };
> +
> +  template<>
> +  struct var_types_storage<var_enum>
> +  {
> +    using type = const char *;
> +  };
> +
> +  /* Helper class used to check if multiple var_types are represented
> +     using the same underlying type.  This class is meant to be instantiated
> +     using any number of var_types, and will be used to assess common
> +     properties of the underlying storage type.
> +
> +     Each template instantiating will define the following static members:

template instantiation?

> +    - value: True if and only if all the var_types are stored on the same
> +    underlying storage type.
> +    - covers_type (var_types t): Function that returns True if and only if the
> +    parameter T is one the templates parameter.
> +    - type: Type alias of the underlying type if value is true, unspecified
> +    otherwise.
> +    */
> +
> +  template<var_types... Ts>
> +  struct var_types_have_same_storage;
> +
> +  /* Specialization of var_types_have_same_storage when instantiated with only
> +     1 template parameter.  */
> +  template<var_types T>
> +  struct var_types_have_same_storage<T>
> +  {
> +    static constexpr bool value = true;
> +
> +    using type = typename var_types_storage<T>::type;
> +
> +    static constexpr bool covers_type (var_types t)
> +    {
> +      return t == T;
> +    }
> +  };
> +
> +  /* Specialization of var_types_have_same_storage when instantiated with
> +     exactly 2 template parameters.  */
> +  template<var_types T, var_types U>
> +  struct var_types_have_same_storage<T, U>
> +  {
> +    static constexpr bool value
> +      = std::is_same<typename var_types_storage<T>::type,
> +		     typename var_types_storage<U>::type>::value;
> +
> +    using type = typename var_types_storage<T>::type;
> +
> +    static constexpr bool covers_type (var_types t)
> +    {
> +      return var_types_have_same_storage<T>::covers_type (t)
> +	     || var_types_have_same_storage<U>::covers_type (t);

When expressions like this span multiple lines, use parenthesis and use
them to align the subsequent lines:

      return (var_types_have_same_storage<T>::covers_type (t)
	      || var_types_have_same_storage<U>::covers_type (t));

Apply the same elsewhere.

> @@ -133,8 +145,7 @@ static SCM unlimited_keyword;
>  
>  static int pascm_is_valid (param_smob *);
>  static const char *pascm_param_type_name (enum var_types type);
> -static SCM pascm_param_value (enum var_types type, void *var,
> -			      int arg_pos, const char *func_name);
> +static SCM pascm_param_value (base_param_ref const &c, int arg_pos, const char *func_name);

Wrap this line.

> @@ -151,10 +162,9 @@ pascm_print_param_smob (SCM self, SCM port, scm_print_state *pstate)
>    if (! pascm_is_valid (p_smob))
>      scm_puts (" {invalid}", port);
>  
> -  gdbscm_printf (port, " %s ", pascm_param_type_name (p_smob->type));
> +  gdbscm_printf (port, " %s ", pascm_param_type_name (p_smob->type ));

Unintended change?

Simon