[PATCH v2 2/5] Use classes to represent MI Command instead of structures
Simon Marchi
simark@simark.ca
Fri May 17 03:12:00 GMT 2019
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
More information about the Gdb-patches
mailing list