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

[Lancelot SIX <lsix@lancelotsix.com>]

> > @@ -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.

Lancelot.

> 
> Simon