From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from lndn.lancelotsix.com (lndn.lancelotsix.com [51.195.220.111]) by sourceware.org (Postfix) with ESMTPS id 80778384F003 for ; Wed, 28 Jul 2021 19:51:00 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 80778384F003 Received: from Plymouth.lan (unknown [IPv6:2a02:390:9086:0:88c6:39a1:a2ed:2505]) by lndn.lancelotsix.com (Postfix) with ESMTPSA id 9E35E83FB7; Wed, 28 Jul 2021 19:50:59 +0000 (UTC) From: Lancelot SIX To: gdb-patches@sourceware.org Cc: Lancelot SIX Subject: [PATCH 4/4] gdb: Param setter return a bool to tell if the value changed Date: Wed, 28 Jul 2021 20:50:43 +0100 Message-Id: <20210728195043.904136-5-lsix@lancelotsix.com> X-Mailer: git-send-email 2.31.1 In-Reply-To: <20210728195043.904136-1-lsix@lancelotsix.com> References: <20210728195043.904136-1-lsix@lancelotsix.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.5.11 (lndn.lancelotsix.com [0.0.0.0]); Wed, 28 Jul 2021 19:50:59 +0000 (UTC) X-Spam-Status: No, score=-11.6 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 28 Jul 2021 19:51:02 -0000 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; } } break; case var_string: { std::string val = std::string (arg); if (new_val != cmd->var.get ()) { cmd->var.get (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] 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 --- gdb/cli/cli-setshow.c | 98 ++++++++++--------------------------------- gdb/command.h | 8 ++-- 2 files changed, 26 insertions(+), 80 deletions(-) diff --git a/gdb/cli/cli-setshow.c b/gdb/cli/cli-setshow.c index 76a0d70a553..7ffa4dfe1cc 100644 --- a/gdb/cli/cli-setshow.c +++ b/gdb/cli/cli-setshow.c @@ -31,17 +31,17 @@ /* Return true if the change of command parameter should be notified. */ -static int -notify_command_param_changed_p (int param_changed, struct cmd_list_element *c) +static bool +notify_command_param_changed_p (bool param_changed, struct cmd_list_element *c) { - if (param_changed == 0) - return 0; + if (!param_changed) + return false; if (c->theclass == class_maintenance || c->theclass == class_deprecated || c->theclass == class_obscure) - return 0; + return false; - return 1; + return true; } @@ -305,7 +305,7 @@ void 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; + bool option_changed = false; gdb_assert (c->type == set_cmd); @@ -353,26 +353,12 @@ do_set_command (const char *arg, int from_tty, struct cmd_list_element *c) *q++ = '\0'; newobj = (char *) xrealloc (newobj, q - newobj); - auto const var = c->var.get (); - if (var != std::string (newobj)) - { - c->var.set (std::string (newobj)); - - option_changed = 1; - } + option_changed = c->var.set (std::string (newobj)); xfree (newobj); } break; case var_string_noescape: - { - auto const var = c->var.get (); - if (var != arg) - { - c->var.set (std::string (arg)); - - option_changed = 1; - } - } + option_changed = c->var.set (std::string (arg)); break; case var_filename: if (*arg == '\0') @@ -398,14 +384,8 @@ do_set_command (const char *arg, int from_tty, struct cmd_list_element *c) else val = xstrdup (""); - auto const var = c->var.get (); - if (var != std::string (val)) - { - c->var.set - (std::string (val)); - - option_changed = 1; - } + option_changed + = c->var.set (std::string (val)); xfree (val); } break; @@ -415,38 +395,19 @@ 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 = 1; - } + 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 = 1; - } - } + 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 = 1; - } - } + option_changed + = c->var.set + (parse_cli_var_uinteger (c->var.type (), &arg, true)); break; case var_integer: case var_zinteger: @@ -476,12 +437,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 = 1; - } + option_changed = c->var.set (val); } break; case var_enum: @@ -494,24 +450,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 = 1; - } + 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 = 1; - } - } + 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 c8510acdf19..3bc4c41e6ab 100644 --- a/gdb/command.h +++ b/gdb/command.h @@ -129,7 +129,7 @@ template struct accessor_sigs { using getter = T (*) (); - using setter = void (*) (T); + using setter = bool (*) (T); }; /* Contains a function to access a parameter. */ @@ -471,7 +471,7 @@ struct base_param_ref template>> - void set (typename detail::var_types_have_same_storage::type v) + bool set (typename detail::var_types_have_same_storage::type v) { /* Check that the current instance is of one of the supported types for this instantiating. */ @@ -483,13 +483,15 @@ struct base_param_ref (this->m_setter); if (setter != nullptr) - (*setter) (v); + return (*setter) (v); else { gdb_assert (!this->empty ()); + bool changed = (this->get () != v); *static_cast< typename detail::var_types_have_same_storage::type *> (this->m_var) = v; + return changed; } } -- 2.31.1