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

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

On 2021-08-10 6:18 p.m., Lancelot SIX wrote:
>>> @@ -231,25 +427,50 @@ struct base_setting
>>>      gdb_assert (var_type_uses<T> (this->m_var_type));
>>>      gdb_assert (!this->empty ());
>>
>> I think this needs to call operator bool, like so:
>>
>>   gdb_assert (*this);
>>
>> Otherwise, the assert fails when using a getter/setter (empty only
>> checks for m_var to be non-NULL, which is false when using
>> getter/setter).
>>
> 
> Thanks for spotting this.  The second assertion should only be on the
> branch that does not use the setter callback.
> 
> I also realized that the 'set' method should use 'get_p<T> () = v', which
> includes this assertion making the one you ran into redundant.
> 
> 
>> I don't find this notation super clear, it might be clearer if we could
>> call a named method instead of operator bool.  Maybe "empty" could mean
>> "does not have a buffer nor getter/setter"?
> 
> I think I got confused about what empty means when I rebased the patch
> and handled the conflicts from the previous iteration. This is a good
> indication that the naming is not as good as it could be.
> 
> 'empty' refers to the fact that there is an underlying buffer register.
> I could just check 'm_var != nullptr' in 'get_p' and 'operator bool()'.
> This is a protected method not meant to be used outside of the class
> anyway.
> 
> The 'bool()' operator is intended to check if a setting is
> valid, i.e. 'get' and 'set' can be called without a guarantied error.
> We could use something like 'valid ()' instead (or 'good ()' if we want
> to mimic iostream).  Not that 'empty' would not work, I think I would
> prefer to read
> 
> 	if (foo.valid ())
> 		use (foo);
> 
> over
> 
> 	if (!foo.empty ())
> 		use (foo);
> 
> I tend to prefer the affirmative version over the double negation.
> 
> I’ll change that for the next iteration.

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.

But otherwise, `valid` sounds good to me as well.

Simon