From 1a98bb552d752d1dbedc8fdae504629d21d776f2 Mon Sep 17 00:00:00 2001 From: Lancelot SIX Date: Tue, 14 Sep 2021 23:37:25 +0100 Subject: [PATCH] gdb: Setting setter return a bool to tell if the value changed 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 ()) { cmd->var.get (new_val); value_changes = 1; } } case var_uinteger: case var_zuinteger: { unsigned int val = parse_cli_var_uinteger (c->var->type (), &arg, true); if (c->var->get () != val) { c->var->set (val); option_changed = true; } } case... /* And so on for all possible var_types. */ } This comparison is done for each possible var_type, which leads to unnecessary logic duplication. In this patch I propose to move all those checks in one place within the setting setter method. This limits the code duplication and simplifies the do_set_command implementation. This patch also changes slightly the way a value change is detected. Instead of comparing the user provided value against the current value of the setting, we compare the value of the setting before and after the set operation. This is meant to handle edge cases where trying to set an unrecognized value would be equivalent to a noop (the actual value remains unchanged). Doing this requires that the original value needs to be copied before the update, which can be non trivial for std::string. There should be no user visible change introduced by this commit. Tested on x86_64 GNU/Linux. [1] https://review.lttng.org/c/binutils-gdb/+/5831/41 Change-Id: If064b9cede3eb56275aacd2b286f74eceb1aed11 --- gdb/cli/cli-setshow.c | 83 +++++++------------------------------------ gdb/command.h | 10 ++++-- 2 files changed, 21 insertions(+), 72 deletions(-) diff --git a/gdb/cli/cli-setshow.c b/gdb/cli/cli-setshow.c index 8d29c0c3fc2..42c2b15b392 100644 --- a/gdb/cli/cli-setshow.c +++ b/gdb/cli/cli-setshow.c @@ -360,26 +360,12 @@ do_set_command (const char *arg, int from_tty, struct cmd_list_element *c) *q++ = '\0'; newobj = (char *) xrealloc (newobj, q - newobj); - const std::string &cur_val = c->var->get (); - if (strcmp (cur_val.c_str(), newobj) != 0) - { - c->var->set (std::string (newobj)); - - option_changed = true; - } + option_changed = c->var->set (std::string (newobj)); xfree (newobj); } break; case var_string_noescape: - { - const std::string &cur_val = c->var->get (); - if (strcmp (cur_val.c_str (), arg) != 0) - { - c->var->set (std::string (arg)); - - option_changed = true; - } - } + option_changed = c->var->set (std::string (arg)); break; case var_filename: if (*arg == '\0') @@ -405,13 +391,8 @@ do_set_command (const char *arg, int from_tty, struct cmd_list_element *c) else val = xstrdup (""); - const std::string &cur_val = c->var->get (); - if (strcmp (cur_val.c_str (), val) != 0) - { - c->var->set (std::string (val)); - - option_changed = true; - } + option_changed + = c->var->set (std::string (val)); xfree (val); } break; @@ -421,39 +402,18 @@ do_set_command (const char *arg, int from_tty, struct cmd_list_element *c) if (val < 0) error (_("\"on\" or \"off\" expected.")); - if (val != c->var->get ()) - { - c->var->set (val); - option_changed = true; - } + option_changed = c->var->set (val); } break; case var_auto_boolean: - { - enum auto_boolean val = parse_auto_binary_operation (arg); - - if (c->var->get () != val) - { - c->var->set (val); - - option_changed = true; - } - } + option_changed = c->var->set (parse_auto_binary_operation (arg)); break; case var_uinteger: case var_zuinteger: - { - unsigned int val - = parse_cli_var_uinteger (c->var->type (), &arg, true); - - if (c->var->get () != val) - { - c->var->set (val); - - option_changed = true; - } - } + option_changed + = c->var->set (parse_cli_var_uinteger (c->var->type (), + &arg, true)); break; case var_integer: case var_zinteger: @@ -483,12 +443,7 @@ do_set_command (const char *arg, int from_tty, struct cmd_list_element *c) || (c->var->type () == var_zinteger && val > INT_MAX)) error (_("integer %s out of range"), plongest (val)); - if (c->var->get () != val) - { - c->var->set (val); - - option_changed = true; - } + option_changed = c->var->set (val); } break; case var_enum: @@ -501,24 +456,12 @@ do_set_command (const char *arg, int from_tty, struct cmd_list_element *c) if (*after != '\0') error (_("Junk after item \"%.*s\": %s"), len, arg, after); - if (c->var->get () != match) - { - c->var->set (match); - - option_changed = true; - } + option_changed = c->var->set (match); } break; case var_zuinteger_unlimited: - { - int val = parse_cli_var_zuinteger_unlimited (&arg, true); - - if (c->var->get () != val) - { - c->var->set (val); - option_changed = true; - } - } + option_changed = c->var->set + (parse_cli_var_zuinteger_unlimited (&arg, true)); break; default: error (_("gdb internal error: bad var_type in do_setshow_command")); diff --git a/gdb/command.h b/gdb/command.h index 2abfbc497b9..7c226f193b8 100644 --- a/gdb/command.h +++ b/gdb/command.h @@ -300,7 +300,9 @@ struct setting return *static_cast (m_var); } - /* Sets the value of the setting to V. + /* Sets the value of the setting to V. Returns true if the setting was + effectively changed, false if the update failed and the setting is left + unchanged. If we have a user-provided setter, use it to set the setting. Otherwise copy the value V to the internally referenced buffer. @@ -310,12 +312,14 @@ struct setting The var_type of the setting must match T. */ template - void set (const T &v) + bool set (const T &v) { /* Check that the current instance is of one of the supported types for this instantiation. */ gdb_assert (var_type_uses (m_var_type)); + const T old_value = this->get (); + if (m_var == nullptr) { gdb_assert (m_setter != nullptr); @@ -324,6 +328,8 @@ struct setting } else *static_cast (m_var) = v; + + return old_value != this->get (); } private: -- 2.30.2