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

[Lancelot SIX <lsix@lancelotsix.com>]

On Mon, Jul 19, 2021 at 10:19:21AM -0400, Simon Marchi wrote:
> > +  setting_variable v { .bool_var = &param_value->boolval };
> > +  return pascm_param_value (type, v, arg_pos, func_name);
> 
> I thought about doing something like this, my only itch is that
> declaration of `setting_variable v`.  It is confusing to see it just
> setting the bool_var field, regardless of the actual type.  I understand
> it works because the pointer value is the same in any case, but to be
> pedantic it should be something like:
> 
>   setting_variable v;
>   switch (type)
>     {
>     case var_boolean:
>       v.bool_var = &param_value->boolval;
>       break;
>     case var_auto_boolean:
>       v.auto_boolean_var = &param_value->autoboolval;
>       break;
>     // and so on
>     }
> 
> It's a bit more verbose, but it's written only once, and it's not like
> new var types are added every week.
> 
> I would be fine changing my patch to use your approach with that change,
> if you are.
> 
> Simon

When sending my comment, I was wondering about suggesting just that.  It
is definitely more verbose, but I would not mind that.  I tend to prefer
verbose code over code duplication.  It is too easy to update only one
of the two implementations and have behaviors diverge.

Just out of curiosity, I have checked with Compiler Explorer[1], and
starting with -01, the entire switch goes away (when using
__builtin_unreachable, without it the switch is dropped but a check is
maintained).  This is surely not a hot code path so this does not
changes much, but it always nice to know the compiler can do the clever
thing.

Lancelot.

[1] https://godbolt.org/z/GdqGno6cq