Re: [PATCH 15/16] gdb: make cmd_list_element var an optional union

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

On 2021-07-14 7:22 p.m., Lancelot SIX wrote:
> On Wed, Jul 14, 2021 at 03:22:41PM -0400, Simon Marchi wrote:
>>
>> I am certainly for this kind of consistency checks, making sure we
>> access the right data the right way.  This is what I did with struct
>> dynamic_prop, for example.
>>
>> I was thinking of adding accessors too, eventually.  I am not very good
>> with advanced C++ templates though, so what I had in mind was more
>> something like the obvious:
>>
>>   struct cmd_list_element
>>   {
>>     int *var_as_int () // or int &
>>     {
>>       gdb_assert (m_var_type == a
>> 		  || m_var_type == b);
>>       return m_var.int_var;
>>     }
>>
>>     unsigned int *var_as_unsigned_int () // or unsigned int &
>>     {
>>       gdb_assert (m_var_type == c
>> 		  || m_var_type == c);
>>       return m_var.unsigned_int_var;
>>     }
>>
>>     // And so on.
>>   };
>>
>> Your last proposition is a bit better in the sense that you ask "give me
>> the variable location for a var_zuinteger".  With mine, you still have
>> to know that a var_zuinteger uses the unsigned int member.  But at
>> least, there's a gdb_assert to catch mistakes at runtime.
>>
>> Whether it's worth the extra complexity... I can't tell, I would have to
>> see the complete patch.  I would also have to see how that fits with the
>> upcoming fix I have for bug 28085.  In the mean time, does it make your
>> life more difficult if we merge my patch?  I would guess not, since
>> you'll need to modify all the call sites (places that access setting
>> variables) anyway.  You can then decide to keep the union or not in your
>> patch.
>>
>> Simon
> 
> I have not yet finished updating all callsites, but here is a
> (hopefully) working patch if you want to see what it looks like.
> 
> I have not had a look at 28085 yet, so I do not yet if my approach gets
> in the way of fixing it.  For the moment, I do not have something
> similar to the gdb_optional you introduce, but it should not be too
> difficult to have something similar.
> 
> Anyway, to not hold back on my account.  I have not read your series
> carefully yet, but it seems a real improvement from the current
> situation.  If it helps fixing a bug, it is even better.  I can rework
> my patch to work on top of this one (at least if what it brings is
> considered worthwhile).
> 
> Lancelot.

I took a quick look, and that seems reasonable to me.  I don't think it
will conflict (in design) with my upcoming patch.  My upcoming patch
makes it so that some settings don't use the storage that is in
cmd_list_element at all, but instead provide some getter/setter
callback.  This is to fix display of settings where the "source of
truth" is not cmd_list_element::var.

With my patch, the code at various places becomes, conceptually:

  if (cmd_list_element::var is set)
    use cmd_list_element::var
  else
    use the getter/setter

I think it would be nice to make the first case (using
cmd_list_element::var for storage) just a specific case of using the
getter/setter.  That is, when we create a setting that uses
cmd_list_element::var for storage, we install a getter and a setter than
get and set the value in cmd_list_element::var.  But I haven't gotten
there yet.  You'll probably have some good ideas for achieving this :).

But before posting that patch, I'd like to decide how the current patch
series will end up, because that will very much affect how the next
patch will look like.

Simon