[v2,2/5] Use classes to represent MI Command instead of structures

  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(-)