Re: [PATCH v3 2/5] Use classes to represent MI Command instead of structures

Pedro Alves
2019-06-18T19:38:14Z

On 5/30/19 2:48 PM, Jan Vrany wrote:
> From: Didier Nadeau <didier.nadeau@gmail.com>
>
> 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 <didier.nadeau@gmail.com>
> + Jan Vrany <jan.vrany@fit.cvut.cz>
> +
> + * 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 <didier.nadeau@gmail.com>
> Jan Vrany <jan.vrany@fit.cvut.cz>
>
> 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 <string>
>
> @@ -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<scoped_restore_tmpl<int>> restore
> + = do_suppress_notification ();
> + this->do_invoke (parse);
> +}
> +
> +gdb::optional<scoped_restore_tmpl<int>>
> +mi_command::do_suppress_notification ()
> +{
> + if (m_suppress_notification != NULL)
> + return scoped_restore_tmpl<int> (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<scoped_restore_tmpl<int>> 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<struct mi_cmd> mi_cmd_up;
> +typedef std::unique_ptr<mi_command> 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<scoped_restore_tmpl<int>> 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