gdb: Setting setter return a bool to tell if the value changedusers/simark/refactor-typesafe-var

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<int> ())
        {
          cmd->var.get<int> (new_val);
          value_changes = 1;
        }
      }
      break;
    case var_string:
      {
        std::string val = std::string (arg);
        if (new_val != cmd->var.get<std::string> ())
        {
          cmd->var.get<std::string> (new_val);
          value_changes = 1;
        }
      }
      break;
    case...
      /* And so on for all possible var_types.  */
    }

This comparison is done for each possible var_type, which leads to
unnecessary code 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).

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

2 files changed, 18 insertions, 71 deletions

diff --git a/gdb/cli/cli-setshow.c b/gdb/cli/cli-setshow.c
index 0f87b92286f..f06c92731e0 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<std::string> ();
-	if (cur_val != std::string (newobj))
-	  {
-	    c->var->set<std::string> (std::string (newobj));
-
-	    option_changed = true;
-	  }
+	option_changed = c->var->set<std::string> (std::string (newobj));
 	xfree (newobj);
       }
       break;
     case var_string_noescape:
-      {
-	const std::string cur_val = c->var->get<std::string> ();
-	if (cur_val != arg)
-	  {
-	    c->var->set<std::string> (std::string (arg));
-
-	    option_changed = true;
-	  }
-      }
+      option_changed = c->var->set<std::string> (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<std::string> ();
-	if (cur_val != std::string (val))
-	  {
-	    c->var->set<std::string> (std::string (val));
-
-	    option_changed = true;
-	  }
+	option_changed
+	  = c->var->set<std::string> (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<bool> ())
-	  {
-	    c->var->set<bool> (val);
 
-	    option_changed = true;
-	  }
+	option_changed = c->var->set<bool> (val);
       }
       break;
     case var_auto_boolean:
-      {
-	enum auto_boolean val = parse_auto_binary_operation (arg);
-
-	if (c->var->get<enum auto_boolean> () != val)
-	  {
-	    c->var->set<enum auto_boolean> (val);
-
-	    option_changed = true;
-	  }
-      }
+      option_changed = c->var->set<enum auto_boolean> (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<unsigned int> () != val)
-	  {
-	    c->var->set<unsigned int> (val);
-
-	    option_changed = true;
-	  }
-      }
+      option_changed
+	= c->var->set<unsigned int> (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<int> () != val)
-	  {
-	    c->var->set<int> (val);
-
-	    option_changed = true;
-	  }
+	option_changed = c->var->set<int> (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<const char *> () != match)
-	  {
-	    c->var->set<const char *> (match);
-
-	    option_changed = true;
-	  }
+	option_changed = c->var->set<const char *> (match);
       }
       break;
     case var_zuinteger_unlimited:
-      {
-	int val = parse_cli_var_zuinteger_unlimited (&arg, true);
-
-	if (c->var->get<int> () != val)
-	  {
-	    c->var->set<int> (val);
-	    option_changed = true;
-	  }
-      }
+      option_changed = c->var->set<int>
+	(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 12626298fda..2c21e68a65d 100644
--- a/gdb/command.h
+++ b/gdb/command.h
@@ -286,12 +286,14 @@ struct setting
 
      The var_type of the setting must match T.  */
   template<typename T>
-  void set (T v)
+  bool set (T v)
   {
     /* Check that the current instance is of one of the supported types for
        this instantiation.  */
     gdb_assert (var_type_uses<T> (this->m_var_type));
 
+    const T old_value = this->get<T> ();
+
     if (m_setter != nullptr)
       {
 	auto setter = reinterpret_cast<setting_setter_ftype<T>> (m_setter);
@@ -302,6 +304,8 @@ struct setting
 	gdb_assert (this->m_var != nullptr);
 	*static_cast<T *> (this->m_var) = v;
       }
+
+    return old_value != this->get<T> ();
   }
 
 protected: