Re: [PATCH 4/4] gdb: Param setter return a bool to tell if the value changed

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

On 2021-07-28 3:50 p.m., Lancelot SIX via Gdb-patches wrote:
> GDB can notify observers when a parameter is changed.
> 
> To do that, do_set_command (in gdb/cli/cli-setshow.c) compares the new
> value against the old one before updating it, and based on that notifies
> observers.  This looks like something like:
> 
>     int valuechanged = 0;
>     switch (cmd->var.type ())
>     {
>     case var_integer:
>       {
>         LONGEST new_val = parse_and_eval_long (arg)
>         if (new_val != cmd->var.get<var_integer> ())
>         {
>           cmd->var.get<var_integer> (new_val);
>           value_changes = 1;
>         }
>       }
>       break;
>     case var_string:
>       {
>         std::string val = std::string (arg);
>         if (new_val != cmd->var.get<var_string> ())
>         {
>           cmd->var.get<var_string> (new_val);
>           value_changes = 1;
>         }
>       }
>       break;
>     case...
>       /* And so on for all possible var_types.  */
>     }
> 
> The main problem is that do_set_command does this comparison
> for each possible var_types it handles, which leads to a significant
> amount of code duplication.
> 
> In this patch I propose to move all those checks in one place within the
> param setter method.  This has two advantages:
> 
> - Code duplication is reduced and the logic is written once within the
>   templated setter.
> - User provided setter can return a flag to indicate if a changed has
>   been made based on its internal logic.  This can be useful for any
>   command on which the set operation might fail.  This use case is
>   inspired by a comment by Simon Marchi[1]

Hmm, now I wonder whether this is really needed.  Getting / setting
settings is typically very cheap.  Would it be ok to invoke the getter,
then the setter and then the getter again, and compare the two values
returned by the getter to determine whether the value has changed?  That
would simplify a little bit the implementation of all setters, as they
would not need to check whether the setting has changed to return the
proper boolean return value.  They would just blindly set the value.  If
some setting is particularly expensive to set, then that particular
setter can always do that check.

Did I mentioned a specific example of set command that could fail, are
you referring to "set non-stop"?  I think it would work here:

 - core code calls getter: gets 0
 - core code calls setter: setter refuses to set value because program
   is running
 - core code calls getter: gets 0
 - core code concludes the setting has not changed

Simon