Re: [PATCH 1/2] gdb: Restore a settings previous value on error during change

[Luis Machado <lgustavo@codesourcery.com>]

On 11/16/2016 12:01 PM, Andrew Burgess wrote:
> * Luis Machado <lgustavo@codesourcery.com> [2016-11-16 08:17:50 -0600]:
>
>> On 11/15/2016 05:02 PM, Andrew Burgess wrote:
>>> * Luis Machado <lgustavo@codesourcery.com> [2016-11-11 18:20:24 -0600]:
>>>>> @@ -151,8 +152,20 @@ do_set_command (const char *arg, int from_tty, struct cmd_list_element *c)
>>>>>  {
>>>>>    /* A flag to indicate the option is changed or not.  */
>>>>>    int option_changed = 0;
>>>>> +  /* Cleanups created to free the previous value of the setting.  */
>>>>> +  struct cleanup *cleanups;
>>>>> +  /* The previous value of the setting.  Restored if the set-hook function
>>>>> +     throws an error.  */
>>>>> +  union
>>>>> +  {
>>>>> +    enum auto_boolean b;
>>>>> +    int i;
>>>>> +    const char *s;
>>>>> +    unsigned int u;
>>>>> +  } prev_value;
>>>>
>>>> I wonder if we can share and potentially simplify the storage mechanism for
>>>> these settings with what struct cmd_list_element already has? As opposed to
>>>> declaring something new just for this particular function?
>>>
>>> I'm not sure how we'd do that.  The 'struct cmd_list_element' holds a
>>> 'void *' pointer to a variable of _some type_ the interpretation of
>>> that 'void *' depends on the 'var_type' field within the 'struct
>>> cmd_list_element'.  But we never actually hold the _value_ of the
>>> setting in the 'struct cmd_list_element'.
>>>
>>> So I think that we will always end up introducing _some_ new structure
>>> to hold the previous value.  I'm open to suggestions though, just
>>> because I can't see the solution doesn't mean it's not there...
>>>
>>
>> What i had in mind was not to declare values with explicit types like the
>> above union but reuse 'enum var_types' with a 'void *' pointer that is the
>> value.
>>
>> Something like:
>>
>> struct prev_value
>> {
>>   void *val;
>>   enum var_type type;
>> };
>>
>> That way we would be able to save the previous value as 'void *' at the top
>> of the function regardless of its type and register the cleanup only
>> once.
>
> I think that you're suggesting saving the _value_ of the setting into
> a 'void *', so a choice of an 'enum auto_boolean', an int (signed and
> unsigned) or a 'char *'.  I guess at the moment they'll all fit, but
> that seems like a fairly ugly bit of code.
>
> Plus we'd still have to have a switch as there's no guarantee that all
> those types are the same size, so when we dereference through the
> cmd_list_element to get the value we'd have to have different code for
> each var_type, otherwise we risk accessing outside the object type.
>
> So we'd end up with:
>
>   struct
>   {
>     void *val;
>     enum var_type type;
>   }  prev_value;
>
>   switch (c->var_type)
>     {
>     case var_string:
>     case var_string_noescape:
>     case var_filename:
>     case var_optional_filename:
>     case var_enum:
>       prev_value.val = (void *)(*(char **) c->var);
>       break;
>
>     case var_boolean:
>     case var_integer:
>     case var_zinteger:
>     case var_zuinteger_unlimited:
>       prev_value.val = (void *)(*(int *) c->var);
>       break;
>
>     case var_auto_boolean:
>       prev_value.val = (void *)(*(enum auto_boolean *) c->var);
>       break;
>
>     case var_uinteger:
>     case var_zuinteger:
>       prev_value.val = (void *)(*(unsigned int *) c->var);
>       break;
>     }
>
> I guess we could collapse the int/unsigned int cases as we know the
> sizes will match, and you'll still need similar code to copy the value
> back into place.
>
> I'm not seeing any particular code saving, nor any improvement in code
> quality there, and any time we use casts to place one item into
> another there's always scope for bugs to creep in....
>
>> Only when we get to the cleanup we decide to free it or not. Then we can use
>> the 'enum var_type' information to free whatever needs to be freed.
>>
>> That way we can get rid of all the type-specific assignments, like here:
>>
>> -	  xfree (*(char **) c->var);
>> +	  prev_value.s = *(const char **) c->var;
>> +	  make_cleanup (xfree, *(char **) c->var);
>>  	  *(char **) c->var = xstrdup (arg);
>>
>> The above gets repeated a few times along the function.
>
> The latest iteration removes the make_cleanup call here.  We now just
> backup the previous value, so that's save a little code.
>
>>
>> Hopefully i managed to make it a bit more clear?
>>
>>>> Since we're already touching this function, it may be worthwhile to get it
>>>> cleaned up a bit if that's not too much effort.
>>>
>>> I can do some clean up if you'd like, I can even make it part of this
>>> patch series, though obviously it'll be a new patch in this series.
>>> But you'll need to give some more specific indication of what you'd
>>> like to see changed....
>>>
>>>>
>>>> I also see a lot of repetition trying to save the old value. If we could get
>>>> it saved at the function's entry, that would save many lines of code, but i
>>>> think this is a nice-to-have and not required for this patch.
>>>
>>> I'm not sure I'd agree with "many", but in the new version I've tried
>>> to reduce the number of new lines added.  As I discuss above, given
>>> that the 'struct cmd_list_element' only has a 'void *' pointer,
>>> backing up the previous value has to be done on a case by case basis.
>>> Again, though, if you have a specific idea in mind that I'm not
>>> seeing, feel free to make a suggestion, I'm happy to change things.
>>>
>>
>> These two comments i've made had the same scope as the first. That is, to
>> use a 'void *' value plus its type and to save the previous value and
>> register the cleanup only once at the top of the function. It was not a
>> separate cleanup idea.
>>
>> In any case, this is just a suggestion.
>
> I've fixed the typos and other minor items you pointed out in your
> initial review (thank you for that), and the second iteration is
> possibly a little cleaner it its use of cleanups.
>
> I don't currently see the benefit / improvement that using a 'void *'
> to store the previous value would offer (over using an union), so I
> have no plan to rewrite in that direction.
>
> I'm always open to being convinced though, if you feel I'm still
> missing the point :)
>
> Thanks,
> Andrew
>

I don't have any further feedback. Hopefully other's will chime in. :-)

Thanks,
Luis