[PATCHv2 2/5] gdb/mi: use std::map for MI commands in mi-cmds.c

[Simon Marchi]

On 2021-12-03 11:29 a.m., Andrew Burgess via Gdb-patches wrote:
> From: Jan Vrany <jan.vrany@labware.com>
>
> This changes the hashmap used in mi-cmds.c from a custom structure to
> std::map.  Not only is replacing a custom container with a standard
> one an improvement, but using std::map will make it easier to
> dynamically add commands; which is something that is planned for a
> later series, where we will allow MI commands to be implemented in
> Python.
>
> There should be no user visible changes after this commit.

LGTM with some minor comments, no need to repost just for that.

> ---
>  gdb/mi/mi-cmds.c | 478 +++++++++++++++++++++++------------------------
>  1 file changed, 230 insertions(+), 248 deletions(-)
>
> diff --git a/gdb/mi/mi-cmds.c b/gdb/mi/mi-cmds.c
> index 8899fdd3a1e..4999c9f81b3 100644
> --- a/gdb/mi/mi-cmds.c
> +++ b/gdb/mi/mi-cmds.c
> @@ -22,283 +22,265 @@
>  #include "top.h"
>  #include "mi-cmds.h"
>  #include "mi-main.h"
> +#include <map>
> +#include <string>
>
> -struct mi_cmd;
> -static struct mi_cmd **lookup_table (const char *command);
> -static void build_table (struct mi_cmd *commands);
> +/* A command held in the MI_CMD_TABLE.  */
>
> -static struct mi_cmd mi_cmds[] =
> -{
> -/* Define a MI command of NAME, and its corresponding CLI command is
> -   CLI_NAME.  */
> -#define DEF_MI_CMD_CLI_1(NAME, CLI_NAME, ARGS_P, CALLED)	\
> -  { NAME, { CLI_NAME, ARGS_P}, NULL, CALLED }
> -#define DEF_MI_CMD_CLI(NAME, CLI_NAME, ARGS_P) \
> -  DEF_MI_CMD_CLI_1(NAME, CLI_NAME, ARGS_P, NULL)
> +typedef std::unique_ptr<struct mi_cmd> mi_cmd_up;

Really a nit: since we're using C++11, I prefer the notation `using =
..`, I find it easier to read (since it looks like a standard
assignment, with the name on the left).

> +/* Create an mi_cmd structure with name NAME.  */
> +
> +static mi_cmd_up
> +create_mi_cmd (const char *name)
>  {
> -  return *lookup_table (command);
> +  mi_cmd_up cmd (new mi_cmd ());
> +  cmd->name = name;
> +  return cmd;
>  }

It looks like this could be a constructor of mi_cmd quite easily,
instead of being a free function.

>
> -/* Used for collecting hash hit/miss statistics.  */
> +/* Create and register a new MI command with an MI specific implementation.
> +   NAME must name an MI command that does not already exist, otherwise an
> +   assertion will trigger.  */
>
> -static struct
> +static void
> +add_mi_cmd_mi (const char *name, mi_cmd_argv_ftype function,
> +	       int *suppress_notification = nullptr)
>  {
> -  int hit;
> -  int miss;
> -  int rehash;
> -} stats;
> +  mi_cmd_up cmd_up = create_mi_cmd (name);
> +
> +  cmd_up->cli.cmd = nullptr;
> +  cmd_up->cli.args_p = 0;
> +  cmd_up->argv_func = function;
> +  cmd_up->suppress_notification = suppress_notification;

... and these could also be initialized in the mi_cmd constructor.  Up
to you to see if you want to make that change, it can also be changed
later.

>
> -/* Look up a command.  */
> +  bool success = insert_mi_cmd_entry (std::move (cmd_up));
> +  gdb_assert (success);
> +}
> +
> +/* Create and register a new MI command implemented on top of a CLI
> +   command.  NAME must name an MI command that does not already exist,
> +   otherwise an assertion will trigger.  */
>
> -static struct mi_cmd **
> -lookup_table (const char *command)
> +static void
> +add_mi_cmd_cli (const char *name, const char *cli_name, int args_p,
> +		int *suppress_notification = nullptr)
>  {
> -  const char *chp;
> -  unsigned int index = 0;
> +  mi_cmd_up cmd_up = create_mi_cmd (name);
>
> -  /* Compute our hash.  */
> -  for (chp = command; *chp; chp++)
> -    {
> -      /* We use a somewhat arbitrary formula.  */
> -      index = ((index << 6) + (unsigned int) *chp) % MI_TABLE_SIZE;
> -    }
> +  cmd_up->cli.cmd = cli_name;
> +  cmd_up->cli.args_p = args_p;
> +  cmd_up->argv_func = nullptr;
> +  cmd_up->suppress_notification = suppress_notification;
>
> -  while (1)
> -    {
> -      struct mi_cmd **entry = &mi_table[index];
> -      if ((*entry) == 0)
> -	{
> -	  /* not found, return pointer to next free. */
> -	  stats.miss++;
> -	  return entry;
> -	}
> -      if (strcmp (command, (*entry)->name) == 0)
> -	{
> -	  stats.hit++;
> -	  return entry;		/* found */
> -	}
> -      index = (index + 1) % MI_TABLE_SIZE;
> -      stats.rehash++;
> -    }
> +  bool success = insert_mi_cmd_entry (std::move (cmd_up));
> +  gdb_assert (success);
>  }
>
> +/* Initialize MI_CMD_TABLE, the global map of MI commands.  */
> +
>  static void
> -build_table (struct mi_cmd *commands)
> +build_table ()

Suggest renaming this to something like "add_builtin_mi_cmds", it would
be more descriptive.

Simon