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

[Simon Marchi <simark@simark.ca>]

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