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

[Lancelot SIX via Gdb-patches <gdb-patches@sourceware.org>]

On Tue, Aug 10, 2021 at 08:22:31AM -0400, Simon Marchi wrote:
> I'm forwarding some comments from Tony Tye, from AMD, who looked at the
> patch, and had some minor comments.
> 
> > @@ -230,7 +226,7 @@ struct cmd_list_element
> >  
> >    /* Pointer to variable affected by "set" and "show".  Doesn't
> >       matter if type is not_set.  */
> > -  void *var = nullptr;
> > +  setting var;
> 
> The comment should probably be updated, this is no longer a pointer.
> Well, for now it contains a pointer, but later in the series it can
> become a getter/setter too.  So maybe just say "Setting affected by
> ...".

Indeed.  I’ll update this and incorporate the other remarks you sent in
your previous email.

> 
> > 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
> > +   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);
> 
> So what will happen if you try to use var_type_uses with an invalid
> template type, link error?  Tony suggested using a static_assert to have
> a compile-time error instead, with an explanation.  I suppose you would
> define a body for this function and put a static_assert (false) in
> there?

With a 'standard' (development) build, this translate to compile-time
error.  I did not pay enough attention, but I implicitly relied on the
fact that the configure script adds '-Werror' by default.  Something
should be in place to fail as early as possible (i.e. compile-time)
whatever compiler options are in use.

We cannot use a plain 'static_assert (false)' here, it will fail even if
this template is never instantiated.  All versions of g++/clang++ I just
tried do trigger consistently:

	$CXX -c -x c++ - <<EOT
	template<typename T>
	void foo ()
	{  static_assert (false, "Invalid instantiation."); }
	EOT

	<stdin>:3:4: error: static_assert failed "Invalid instantiation."
	{  static_assert (false, "Invalid instantiation."); }
	   ^              ~~~~~
	   1 error generated.

I could use something similar to this:

	template<typename T>
	inline bool var_type_uses (var_types var_type ATTRIBUTE_UNUSED)
	{
	  static_assert (sizeof(T) == 0,
			 "Invalid type used to instantiate 'var_type_uses'. "
			 "Use one of the types associated with enum var_types.");
	  return false;
	}


It is not the prettiest way of writing an assertion, but it is the only
one I am aware of that can work in this situation.

Would this be OK?

Lancelot.