Re: [PATCH v2 3/4] gdb: Have setter and getter callbacks for settings

[Lancelot SIX <lsix@lancelotsix.com>]

On Thu, Aug 19, 2021 at 05:03:32PM -0400, Simon Marchi wrote:
> On 2021-08-11 4:00 p.m., Lancelot SIX wrote:
> >> Another option would be to use
> >>
> >>   gdb::optional<setting> var;
> >>
> >> in cmd_list_element, and make it such that a setting can never exist in
> >> an invalid state.  Two constructor would exist, one to construct a
> >> setting with a pointer to a buffer and another with the setter/getter.
> >> There would be no need (I think) for set_p and set_accessors.
> > 
> > This approach would be a problem within param_smob
> > (gdb/guile/scm-param.c). This struct is memset initialized, so we cannot
> > have one of its member which is not a PODType (see the redefinition of
> > memset in gdbsupport/poison.h).  We could have a pointer to a properly
> > constructed setting within a param_smob plus some manual memory
> > management, but I am not sure it is worth it.
> 
> Tony Tye sent me some comments about this patch, which made me
> reconsider the approach a bit.  I don't see the problem with
> Guile / param_smob.  In your patch, setting_wrapper is just a transient
> object, it exists just for a moment to make the glue between a setting
> (which can be handled by the common command code) and a param_smob.  But
> it never needs to be stored anywhere (in a POD type or otherwise).
> 
> I implemented this approach of using gdb::optional and constructors, and
> it seems to work pretty well.  I prefer it, because, as mentioned
> before, the setting object can't exist in an invalid state.  And that
> is one less thing to worry about.

Hi,

I think that in my first iteration on this series I had a non trivial
ctor and it triggered a problem around the guile code.  I did not dig
too much in this direction and did not reconsider later.  If
gdb::optional can be used, I also prefer the option where we only use
non trivial constructors and ensure that the object can only exist in a
valid state.

> 
> The other comment was that the unions setting_getter and setting_setter
> plus all the get_setting_setter / get_setting_getter implementations
> were not really needed but add a lot of code.  Tony suggested to save
> the getter and setter function pointers to type-erased variables, just
> like we do for m_var.  And I don't think we lose any safety, as
> var_type_uses already validates that we don't pass some invalid type T,
> and that T matches var_type.  That removes a lot of boilerplate.  I
> implemented this, and stored the getters / setters as `void (*) ()`.
> Storing them as `void *` wouldn't work, because you can't freely convert
> function pointers to data pointers and vice versa.
> 

I did use a union there because I could not use 'void *'.  I did not
consider to cast to 'void (*) ()' but indeed, it will be a lot less
code, more consistent with the type erasure done with m_var and as safe
as the union option.  Only good reasons to go for this approach.

> I started from your users/lsix/refactor-typesafe-var, made changes to
> the various patches, and uploaded it to
> users/simark/refactor-typesafe-var.
> 

I’ll have a look as soon as I can.  Thanks for doing this!

> Let me know what you think.
>

I like both of the changes you have suggested. They are improvements
over my patch set.  Thanks a lot to you and Tony for the reviews,
suggestions and improvements.

Lancelot.

> Simon