From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 29762 invoked by alias); 18 Jun 2019 19:38:14 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 29754 invoked by uid 89); 18 Jun 2019 19:38:14 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.1 spammy=Official, losing X-HELO: mail-wr1-f65.google.com Received: from mail-wr1-f65.google.com (HELO mail-wr1-f65.google.com) (209.85.221.65) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 18 Jun 2019 19:38:10 +0000 Received: by mail-wr1-f65.google.com with SMTP id v14so782263wrr.4 for ; Tue, 18 Jun 2019 12:38:10 -0700 (PDT) Return-Path: Received: from ?IPv6:2001:8a0:f913:f700:56ee:75ff:fe8d:232b? ([2001:8a0:f913:f700:56ee:75ff:fe8d:232b]) by smtp.gmail.com with ESMTPSA id a67sm3405238wmh.40.2019.06.18.12.38.07 (version=TLS1_3 cipher=AEAD-AES128-GCM-SHA256 bits=128/128); Tue, 18 Jun 2019 12:38:07 -0700 (PDT) Subject: Re: [PATCH v3 2/5] Use classes to represent MI Command instead of structures To: Jan Vrany , gdb-patches@sourceware.org References: <20190128124101.26243-1-jan.vrany@fit.cvut.cz> <20190530134850.3236-3-jan.vrany@fit.cvut.cz> Cc: Didier Nadeau From: Pedro Alves Message-ID: <0f3e33c4-01a8-72e6-8bdf-863b85ac1006@redhat.com> Date: Tue, 18 Jun 2019 19:38:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1 MIME-Version: 1.0 In-Reply-To: <20190530134850.3236-3-jan.vrany@fit.cvut.cz> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2019-06/txt/msg00352.txt.bz2 On 5/30/19 2:48 PM, Jan Vrany wrote: > From: Didier Nadeau > > This commit changes the infrastructure of mi-cmds.h and associated > files to use classes instead of structure to populate the hashmap > containing the commands. > > The base class is virtual and there is one subclass MI commands > implemented with pure MI implementation and another for MI commands > implemented over a CLI command. > > Logic for suppress_notification and parsing of ARGV/ARGC has been moved > to the classes implementation. > > gdb/ChangeLog: > > * mi/mi-cmds.c (create_mi_cmd): Remove. > (mi_command::mi_command): New function. > (mi_command::do_suppress_notification): New function. > (mi_command::invoke): New function. > (mi_command_mi::mi_command_mi): New function. > (mi_command_mi::do_invoke): New function. > (mi_command_cli::mi_command_cli): New function. > (mi_command_cli::do_invoke): New function. > (mi_cmd_lookup): Change return type. > * mi/mi-cmds.h (struct mi_cli): Remove. > (struct mi_cmd): Remove. > (class mi_command): New class. > (class mi_command_mi): New class. > (class mi_command_cli): New class. > (mi_cmd_loopkup): Change return type. > * mi/mi-main.c (mi_execute_cli_command): Remove declaration. > (mi_execute_command): Remove suppress_notification handling. > (mi_cmd_execute): Remove call to argv_func. > (mi_cmd_execute): Remove call to mi_execute_cli_command. > (mi_cmd_execute): New call to mi_command::invoke. > * mi/mi-main.h (mi_execute_cli_command): New declaration. > * mi/mi-parse.c (mi_parse): Remove call to mi_parse_argv. > * mi/mi-parse.h (struct mi_parse): Remove field struct mi_cmd. > (struct mi_parse): New field class mi_command. > (mi_parse_argv): New declaration. > --- > gdb/ChangeLog | 29 +++++++++++++ > gdb/mi/mi-cmd-info.c | 2 +- > gdb/mi/mi-cmds.c | 100 ++++++++++++++++++++++++++++++------------- > gdb/mi/mi-cmds.h | 75 ++++++++++++++++++++++---------- > gdb/mi/mi-main.c | 22 +--------- > gdb/mi/mi-main.h | 1 + > gdb/mi/mi-parse.c | 18 ++------ > gdb/mi/mi-parse.h | 6 ++- > 8 files changed, 164 insertions(+), 89 deletions(-) > > diff --git a/gdb/ChangeLog b/gdb/ChangeLog > index 8e964c0adb..994ad947bd 100644 > --- a/gdb/ChangeLog > +++ b/gdb/ChangeLog > @@ -1,3 +1,32 @@ > +2019-05-02 Didier Nadeau > + Jan Vrany > + > + * mi/mi-cmds.c (create_mi_cmd): Remove. > + (mi_command::mi_command): New function. > + (mi_command::do_suppress_notification): New function. > + (mi_command::invoke): New function. > + (mi_command_mi::mi_command_mi): New function. > + (mi_command_mi::do_invoke): New function. > + (mi_command_cli::mi_command_cli): New function. > + (mi_command_cli::do_invoke): New function. > + (mi_cmd_lookup): Change return type. > + * mi/mi-cmds.h (struct mi_cli): Remove. > + (struct mi_cmd): Remove. > + (class mi_command): New class. > + (class mi_command_mi): New class. > + (class mi_command_cli): New class. > + (mi_cmd_loopkup): Change return type. > + * mi/mi-main.c (mi_execute_cli_command): Remove declaration. > + (mi_execute_command): Remove suppress_notification handling. > + (mi_cmd_execute): Remove call to argv_func. > + (mi_cmd_execute): Remove call to mi_execute_cli_command. > + (mi_cmd_execute): New call to mi_command::invoke. > + * mi/mi-main.h (mi_execute_cli_command): New declaration. > + * mi/mi-parse.c (mi_parse): Remove call to mi_parse_argv. > + * mi/mi-parse.h (struct mi_parse): Remove field struct mi_cmd. > + (struct mi_parse): New field class mi_command. > + (mi_parse_argv): New declaration. > + > 2019-05-02 Didier Nadeau > Jan Vrany > > diff --git a/gdb/mi/mi-cmd-info.c b/gdb/mi/mi-cmd-info.c > index 8645fac294..3d8bd68264 100644 > --- a/gdb/mi/mi-cmd-info.c > +++ b/gdb/mi/mi-cmd-info.c > @@ -67,7 +67,7 @@ void > mi_cmd_info_gdb_mi_command (const char *command, char **argv, int argc) > { > const char *cmd_name; > - struct mi_cmd *cmd; > + mi_command *cmd; > struct ui_out *uiout = current_uiout; > > /* This command takes exactly one argument. */ > diff --git a/gdb/mi/mi-cmds.c b/gdb/mi/mi-cmds.c > index 13db22afaf..2d8863c5f9 100644 > --- a/gdb/mi/mi-cmds.c > +++ b/gdb/mi/mi-cmds.c > @@ -22,6 +22,7 @@ > #include "top.h" > #include "mi-cmds.h" > #include "mi-main.h" > +#include "mi-parse.h" > #include > #include > > @@ -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 ()); > > - std::string name (command->name); > + const std::string &name = command->name (); > > if (mi_cmd_table.find (name) != mi_cmd_table.end ()) > return false; > @@ -48,32 +49,16 @@ insert_mi_cmd_entry (mi_cmd_up command) > return true; > } > > -/* Create an mi_cmd structure with name NAME. */ > - > -static mi_cmd_up > -create_mi_cmd (const char *name) > -{ > - mi_cmd_up cmd (new mi_cmd ()); > - > - cmd->name = name; > - > - return cmd; > -} > - > /* 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_cmd_up command (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 (command)); > gdb_assert (success); > } > > @@ -83,17 +68,72 @@ 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_cmd_up command (new mi_command_cli (name, cli_name, args_p, > + suppress_notification)); > > - 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 (command)); > gdb_assert (success); > } > > +/* See mi-cmds.h */ Missing period, and missing empty line between the comment and the function. > +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); > +} > + > +gdb::optional> > +mi_command::do_suppress_notification () > +{ > + if (m_suppress_notification != NULL) > + return scoped_restore_tmpl (m_suppress_notification, 1); > + else > + return {}; > +} > + > +mi_command_mi::mi_command_mi (const char *name, mi_cmd_argv_ftype func, > + int *suppress_notification) > + : mi_command (name, suppress_notification), > + m_argv_function (func) > +{ > + gdb_assert (func != NULL); > +} > + > +void > +mi_command_mi::do_invoke (struct mi_parse *parse) > +{ > + mi_parse_argv (parse->args, parse); > + > + if (parse->argv == NULL) > + error (_("Problem parsing arguments: %s %s"), parse->command, parse->args); > + > + this->m_argv_function (parse->command, parse->argv, parse->argc); > +} > + > +mi_command_cli::mi_command_cli (const char *name, const char *cli_name, > + int args_p, int *suppress_notification) > + : mi_command (name, suppress_notification), > + m_cli_name (cli_name), > + m_args_p (args_p) > +{} > + > +void > +mi_command_cli::do_invoke (struct mi_parse *parse) > +{ > + mi_execute_cli_command (this->m_cli_name.c_str (), this->m_args_p, > + parse->args); > +} > + > +/* Initialize the available MI commands. */ > + > static void > build_table () > { > @@ -126,7 +166,7 @@ build_table () > add_mi_cmd_mi ("catch-exception", mi_cmd_catch_exception, > &mi_suppress_notification.breakpoint); > add_mi_cmd_mi ("catch-handlers", mi_cmd_catch_handlers, > - &mi_suppress_notification.breakpoint); > + &mi_suppress_notification.breakpoint); You should fix this whitespace issue in patch #1, i.e., make patch #1 not introduce the problem in the first place. > add_mi_cmd_mi ("catch-load", mi_cmd_catch_load, > &mi_suppress_notification.breakpoint); > add_mi_cmd_mi ("catch-unload", mi_cmd_catch_unload, > @@ -244,7 +284,7 @@ build_table () > > /* See mi-cmds.h. */ > > -struct mi_cmd * > +mi_command * > mi_cmd_lookup (const char *command) > { > gdb_assert (command != NULL); > diff --git a/gdb/mi/mi-cmds.h b/gdb/mi/mi-cmds.h > index becd788d0f..f1b4728bde 100644 > --- a/gdb/mi/mi-cmds.h > +++ b/gdb/mi/mi-cmds.h > @@ -127,38 +127,69 @@ extern mi_cmd_argv_ftype mi_cmd_enable_frame_filters; > extern mi_cmd_argv_ftype mi_cmd_var_set_update_range; > extern mi_cmd_argv_ftype mi_cmd_complete; > > -/* Description of a single command. */ > +/* mi_command base virtual class. */ Say "abstract base class" instead of "base virtual class". "virtual class" has a different meaning in C++, it refers to virtual inheritance, which you're not using here. How about: /* Abstract base class inherited by MI commands with either a pure MI implementation or implemented on top of CLI commands. */ > > -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 () {}; Spurious ";" after the dtor's body. > + > + 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; These two methods should be documented here. > + > +private: > + > + /* The name of the command. */ > + std::string m_name; > + > + /* Pointer to integer to set during command's invocation. */ > + int *m_suppress_notification; > }; > > -struct mi_cmd > +/* MI command with a pure MI implementation. */ > + > +class mi_command_mi : public mi_command > { > - /* Official name of the command. */ > - const char *name; > - /* The corresponding CLI command that can be used to implement this > - MI command (if cli.lhs is non NULL). */ > - struct mi_cli cli; > - /* If non-null, the function implementing the MI command. */ > - mi_cmd_argv_ftype *argv_func; > - /* If non-null, the pointer to a field in > - 'struct mi_suppress_notification', which will be set to true by MI > - command processor (mi-main.c:mi_cmd_execute) when this command is > - being executed. It will be set back to false when command has been > - executed. */ > - int *suppress_notification; > +public: > + mi_command_mi (const char *name, mi_cmd_argv_ftype func, > + int *suppress_notification); The ctor's arguments should be documented. Particularly, what's FUNC ? > + > +protected: > + void do_invoke(struct mi_parse *parse) override; Missing space before open parens. > + > +private: > + mi_cmd_argv_ftype *m_argv_function; Please document this. > +}; > + > +/* 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, > + int *suppress_notification); > + > +protected: > + void do_invoke(struct mi_parse *parse) override; Missing space. > + > +private: > + std::string m_cli_name; > + int m_args_p; Please document these. Some of these fields were documented before but it seems like you're just dropping the comments. E.g., we can see this talking about args_p in the context above: > - /* 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; Please make sure that we're not losing such comments. > }; > > -typedef std::unique_ptr mi_cmd_up; > +typedef std::unique_ptr mi_cmd_up; > > /* Lookup a command in the MI command table. */ > > -extern struct mi_cmd *mi_cmd_lookup (const char *command); > +extern mi_command *mi_cmd_lookup (const char *command); > > /* Debug flag */ > extern int mi_debug_p; > diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c > index 4921c13528..921d0b5a03 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 *, > @@ -1952,11 +1949,6 @@ mi_execute_command (const char *cmd, int from_tty) > { > ptid_t previous_ptid = inferior_ptid; > > - gdb::optional> restore_suppress; > - > - if (command->cmd != NULL && command->cmd->suppress_notification != NULL) > - restore_suppress.emplace (command->cmd->suppress_notification, 1); > - > command->token = token; > > if (do_timings) > @@ -2095,18 +2087,8 @@ 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) > - { > - /* 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); > - } > + if (parse->cmd != NULL) > + parse->cmd->invoke (parse); > else > { > /* FIXME: DELETE THIS. */ > diff --git a/gdb/mi/mi-main.h b/gdb/mi/mi-main.h > index 1986228d22..90d54a6eed 100644 > --- a/gdb/mi/mi-main.h > +++ b/gdb/mi/mi-main.h > @@ -54,6 +54,7 @@ struct mi_suppress_notification > }; > extern struct mi_suppress_notification mi_suppress_notification; > > +void mi_execute_cli_command (const char *cmd, int args_p, const char *args); > /* Implementation of -fix-multi-location-breakpoint-output. */ Write "extern void" ... like the other declarations around this. Also, please add an empty line between the new declaration and the unrelated comment just below. Thanks, Pedro Alves