This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 1/2] gdb: Restore a settings previous value on error during change
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. 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.
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.