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

Simon Marchi
2019-05-17T03:12:49Z

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<scoped_restore_tmpl<int>> restore
> + = do_suppress_notification ();
> + this->do_invoke(parse);

Missing space.

> +}
> +
> +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 gdb::optional<scoped_restore_tmpl<int>> ();
> + }

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<scoped_restore_tmpl<int>> 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<scoped_restore_tmpl<int>> 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 <simon.marchi@polymtl.ca>
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<scoped_restore_tmpl<int>> restore
= do_suppress_notification ();
- this->do_invoke(parse);
+ this->do_invoke (parse);
}

gdb::optional<scoped_restore_tmpl<int>>
@@ -96,9 +96,7 @@ mi_command::do_suppress_notification ()
if (m_suppress_notification != NULL)
return scoped_restore_tmpl<int> (m_suppress_notification, 1);
else
- {
- return gdb::optional<scoped_restore_tmpl<int>> ();
- }
+ 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<scoped_restore_tmpl<int>> 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<scoped_restore_tmpl<int>> 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_command> 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<scoped_restore_tmpl<int>> 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. */
--
2.21.0