From patchwork Fri May 17 03:12:43 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Simon Marchi X-Patchwork-Id: 32735 Received: (qmail 9412 invoked by alias); 17 May 2019 03:12:49 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 9042 invoked by uid 89); 17 May 2019 03:12:48 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-18.0 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, SPF_HELO_PASS, SPF_PASS autolearn=ham version=3.3.1 spammy=H*Ad:D*ca, suppression, 13060 X-HELO: simark.ca Received: from simark.ca (HELO simark.ca) (158.69.221.121) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 17 May 2019 03:12:46 +0000 Received: from [10.0.0.11] (unknown [192.222.164.54]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 934E81E623; Thu, 16 May 2019 23:12:43 -0400 (EDT) Subject: Re: [PATCH v2 2/5] Use classes to represent MI Command instead of structures To: Jan Vrany , gdb-patches@sourceware.org Cc: Didier Nadeau References: <20190418152337.6376-1-jan.vrany@fit.cvut.cz> <20190514112418.24091-3-jan.vrany@fit.cvut.cz> From: Simon Marchi Message-ID: <7dffd6b7-855c-2dea-8cee-64d895b38bff@simark.ca> Date: Thu, 16 May 2019 23:12:43 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.6.1 MIME-Version: 1.0 In-Reply-To: <20190514112418.24091-3-jan.vrany@fit.cvut.cz> Hi Jan, This looks ok to me, I have noted a few styling issues, I fixed them as I went through the code, so see the patch at the end which you can apply to your patch. > @@ -36,9 +37,9 @@ static bool > insert_mi_cmd_entry (mi_cmd_up command) > { > gdb_assert (command != NULL); > - gdb_assert (command->name != NULL); > + gdb_assert (! command->name ().empty ()); Remove space after !. > /* Create and register a new MI command with a pure MI implementation. */ > > static void > add_mi_cmd_mi (const char *name, mi_cmd_argv_ftype function, > int *suppress_notification = NULL) > { > - mi_cmd_up cmd_up = create_mi_cmd (name); > - > - cmd_up->cli.cmd = NULL; > - cmd_up->cli.args_p = 0; > - cmd_up->argv_func = function; > - cmd_up->suppress_notification = suppress_notification; > + mi_command *micommand = new mi_command_mi (name, function, > + suppress_notification); > > - bool success = insert_mi_cmd_entry (std::move (cmd_up)); > + bool success = insert_mi_cmd_entry (std::move (mi_cmd_up (micommand))); It would make more sense to make the local variable an mi_cmd_up. > gdb_assert (success); > } > > @@ -83,17 +68,74 @@ static void > add_mi_cmd_cli (const char *name, const char *cli_name, int args_p, > int *suppress_notification = NULL) > { > - mi_cmd_up cmd_up = create_mi_cmd (name); > + mi_command *micommand = new mi_command_cli (name, cli_name, args_p, > + suppress_notification); Likewise. > > - cmd_up->cli.cmd = cli_name; > - cmd_up->cli.args_p = args_p; > - cmd_up->argv_func = NULL; > - cmd_up->suppress_notification = suppress_notification; > - > - bool success = insert_mi_cmd_entry (std::move (cmd_up)); > + bool success = insert_mi_cmd_entry (std::move (mi_cmd_up (micommand))); > gdb_assert (success); > } > > +/* See mi-cmds.h */ > +mi_command::mi_command (const char *name, int *suppress_notification) > + : m_name (name), > + m_suppress_notification (suppress_notification) > +{} > + > + > +void > +mi_command::invoke (struct mi_parse *parse) > +{ > + gdb::optional> restore > + = do_suppress_notification (); > + this->do_invoke(parse); Missing space. > +} > + > +gdb::optional> > +mi_command::do_suppress_notification () > +{ > + if (m_suppress_notification != NULL) > + return scoped_restore_tmpl (m_suppress_notification, 1); > + else > + { > + return gdb::optional> (); > + } Remove the curly braces. Also, this can be shortened to "return {};". > @@ -126,38 +126,69 @@ extern mi_cmd_argv_ftype mi_cmd_enable_pretty_printing; > extern mi_cmd_argv_ftype mi_cmd_enable_frame_filters; > extern mi_cmd_argv_ftype mi_cmd_var_set_update_range; > > -/* Description of a single command. */ > +/* mi_command base virtual class. */ > > -struct mi_cli > +class mi_command > { > - /* Corresponding CLI command. If ARGS_P is non-zero, the MI > - command's argument list is appended to the CLI command. */ > - const char *cmd; > - int args_p; > + public: > + mi_command (const char *name, int *suppress_notification); > + virtual ~mi_command () {}; Use the default destructor: virtual ~mi_command () = default; > + > + const std::string &name () > + { return m_name; } > + > + /* Execute the MI command. */ > + void invoke (struct mi_parse *parse); > + > + protected: > + gdb::optional> do_suppress_notification (); > + virtual void do_invoke(struct mi_parse *parse) = 0; It would be useful to have some short doc for these. Also, I think do_invoke could be private. > + > + private: > + > + /* The name of the command. */ > + std::string m_name; > + > + /* Pointer to integer to set during command's invocation. */ > + int *m_suppress_notification; > }; Unindent the content of the class: the public/private should be at the same column as the "class" keyword. > diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c > index 17ca807471..481f09f799 100644 > --- a/gdb/mi/mi-main.c > +++ b/gdb/mi/mi-main.c > @@ -90,9 +90,6 @@ int running_result_record_printed = 1; > int mi_proceeded; > > static void mi_cmd_execute (struct mi_parse *parse); > - > -static void mi_execute_cli_command (const char *cmd, int args_p, > - const char *args); > static void mi_execute_async_cli_command (const char *cli_command, > char **argv, int argc); > static bool register_changed_p (int regnum, readonly_detached_regcache *, > @@ -1954,9 +1951,6 @@ mi_execute_command (const char *cmd, int from_tty) > > gdb::optional> restore_suppress; This ends up ununsed and can be removed. > > - if (command->cmd != NULL && command->cmd->suppress_notification != NULL) > - restore_suppress.emplace (command->cmd->suppress_notification, 1); > - > command->token = token; > > if (do_timings) > @@ -2095,17 +2089,9 @@ mi_cmd_execute (struct mi_parse *parse) > > current_context = parse; > > - if (parse->cmd->argv_func != NULL) > - { > - parse->cmd->argv_func (parse->command, parse->argv, parse->argc); > - } > - else if (parse->cmd->cli.cmd != 0) > + if (parse->cmd != NULL) > { > - /* FIXME: DELETE THIS. */ > - /* The operation is still implemented by a cli command. */ > - /* Must be a synchronous one. */ > - mi_execute_cli_command (parse->cmd->cli.cmd, parse->cmd->cli.args_p, > - parse->args); > + parse->cmd->invoke (parse); Remove resulting extra curly braces. Here's the diff that should fix pretty much all these comments. From 6e7b648b887e59eaadb01919ca0d665dae2b3e19 Mon Sep 17 00:00:00 2001 From: Simon Marchi Date: Thu, 16 May 2019 22:00:00 -0400 Subject: [PATCH] fixup patch 2 --- gdb/mi/mi-cmds.c | 21 ++++++++--------- gdb/mi/mi-cmds.h | 60 +++++++++++++++++++++++++----------------------- gdb/mi/mi-main.c | 6 +---- 3 files changed, 41 insertions(+), 46 deletions(-) diff --git a/gdb/mi/mi-cmds.c b/gdb/mi/mi-cmds.c index 44f3813fdc9a..7702011e39df 100644 --- a/gdb/mi/mi-cmds.c +++ b/gdb/mi/mi-cmds.c @@ -37,7 +37,7 @@ static bool insert_mi_cmd_entry (mi_cmd_up command) { gdb_assert (command != NULL); - gdb_assert (! command->name ().empty ()); + gdb_assert (!command->name ().empty ()); const std::string &name = command->name (); @@ -55,10 +55,9 @@ static void add_mi_cmd_mi (const char *name, mi_cmd_argv_ftype function, int *suppress_notification = NULL) { - mi_command *micommand = new mi_command_mi (name, function, - suppress_notification); + mi_cmd_up command (new mi_command_mi (name, function, suppress_notification)); - bool success = insert_mi_cmd_entry (std::move (mi_cmd_up (micommand))); + bool success = insert_mi_cmd_entry (std::move (command)); gdb_assert (success); } @@ -68,14 +67,15 @@ static void add_mi_cmd_cli (const char *name, const char *cli_name, int args_p, int *suppress_notification = NULL) { - mi_command *micommand = new mi_command_cli (name, cli_name, args_p, - suppress_notification); + mi_cmd_up command (new mi_command_cli (name, cli_name, args_p, + suppress_notification)); - bool success = insert_mi_cmd_entry (std::move (mi_cmd_up (micommand))); + bool success = insert_mi_cmd_entry (std::move (command)); gdb_assert (success); } /* See mi-cmds.h */ + mi_command::mi_command (const char *name, int *suppress_notification) : m_name (name), m_suppress_notification (suppress_notification) @@ -87,7 +87,7 @@ mi_command::invoke (struct mi_parse *parse) { gdb::optional> restore = do_suppress_notification (); - this->do_invoke(parse); + this->do_invoke (parse); } gdb::optional> @@ -96,9 +96,7 @@ mi_command::do_suppress_notification () if (m_suppress_notification != NULL) return scoped_restore_tmpl (m_suppress_notification, 1); else - { - return gdb::optional> (); - } + return {}; } mi_command_mi::mi_command_mi (const char *name, mi_cmd_argv_ftype func, @@ -303,4 +301,3 @@ _initialize_mi_cmds (void) { build_table (); } - diff --git a/gdb/mi/mi-cmds.h b/gdb/mi/mi-cmds.h index fc432a16b9cc..12b9696c5f41 100644 --- a/gdb/mi/mi-cmds.h +++ b/gdb/mi/mi-cmds.h @@ -130,58 +130,60 @@ extern mi_cmd_argv_ftype mi_cmd_var_set_update_range; class mi_command { - public: - mi_command (const char *name, int *suppress_notification); - virtual ~mi_command () {}; +public: + mi_command (const char *name, int *suppress_notification); + virtual ~mi_command () = default; - const std::string &name () - { return m_name; } + const std::string &name () + { return m_name; } - /* Execute the MI command. */ - void invoke (struct mi_parse *parse); + /* Execute the MI command. */ + void invoke (struct mi_parse *parse); - protected: - gdb::optional> do_suppress_notification (); - virtual void do_invoke(struct mi_parse *parse) = 0; +protected: + /* Set the notification suppression flag for the command. Return a + scoped_restore that undoes it. */ + gdb::optional> do_suppress_notification (); - private: +private: + /* Implementation of the behavior of the MI command, to be implemented by + child classes. */ + virtual void do_invoke (struct mi_parse *parse) = 0; - /* The name of the command. */ - std::string m_name; + /* The name of the command. */ + std::string m_name; - /* Pointer to integer to set during command's invocation. */ - int *m_suppress_notification; + /* Pointer to integer to set during command's invocation. */ + int *m_suppress_notification; }; /* MI command with a pure MI implementation. */ class mi_command_mi : public mi_command { - public: - mi_command_mi (const char *name, mi_cmd_argv_ftype func, - int *suppress_notification); +public: + mi_command_mi (const char *name, mi_cmd_argv_ftype func, + int *suppress_notification); - protected: - void do_invoke(struct mi_parse *parse) override; +private: + virtual void do_invoke (struct mi_parse *parse) override; - private: - mi_cmd_argv_ftype *m_argv_function; + mi_cmd_argv_ftype *m_argv_function; }; /* MI command implemented on top of a CLI command. */ class mi_command_cli : public mi_command { - public: - mi_command_cli (const char *name, const char *cli_name, int args_p, +public: + mi_command_cli (const char *name, const char *cli_name, int args_p, int *suppress_notification); - protected: - void do_invoke(struct mi_parse *parse) override; +private: + virtual void do_invoke (struct mi_parse *parse) override; - private: - std::string m_cli_name; - int m_args_p; + std::string m_cli_name; + int m_args_p; }; typedef std::unique_ptr mi_cmd_up; diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c index 754091656248..ccb7bf45bdf5 100644 --- a/gdb/mi/mi-main.c +++ b/gdb/mi/mi-main.c @@ -1949,8 +1949,6 @@ mi_execute_command (const char *cmd, int from_tty) { ptid_t previous_ptid = inferior_ptid; - gdb::optional> restore_suppress; - command->token = token; if (do_timings) @@ -2090,9 +2088,7 @@ mi_cmd_execute (struct mi_parse *parse) current_context = parse; if (parse->cmd != NULL) - { - parse->cmd->invoke (parse); - } + parse->cmd->invoke (parse); else { /* FIXME: DELETE THIS. */