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

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

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.

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 started from your users/lsix/refactor-typesafe-var, made changes to
the various patches, and uploaded it to
users/simark/refactor-typesafe-var.

Let me know what you think.

Simon