[RFA 1/4] Implement user defined prefix.

[Simon Marchi]

On 2019-09-29 4:54 p.m., Philippe Waroquiers wrote:
> This patch adds the new 'prefix-define' command that creates (or mark an
> existing user defined command) as a prefix command.
> This approach was preferred compared to add a -prefix option to
> 'define' command : with prefix-define, a command can be defined and
> afterwards marked as a prefix.  Also, it is easier to define a
> 'prefix' only command in one operation.

It's a bit a detail, but one we can't change later on, so I'll mention it
anyway.  I think "define-prefix" would fit more with the existing GDB
commands, which seem to be mostly "ACTION-OBJECT".  For example, add-inferior,
compare-sections, generate-core-file, queue-signal.  I found flash-erase as
a counter example, but that's about it.

> This patch also adds completers for the 'define' and 'document' commands.
> This makes it easier for the user to type the prefixes for 'define'
> and type the documented command name for 'document'.
> 
> gdb/ChangeLog
> YYYY-MM-DD  Philippe Waroquiers  <philippe.waroquiers@skynet.be>
> 
> 	* cli/cli-script.c (do_define_command): Ensure a redefined
> 	prefix command is kept as a prefix command.
> 	(prefix_define_command): New function.
> 	(show_user_1): Report user defined prefixes.
> 	(_initialize_cli_script):  Create the new 'prefix-define' command.
> 	Add completers for 'define' and 'document'.
> 	* top.c (execute_command):  If command is a user-defined prefix only
> 	command, report the list of commands for this prefix command.
> ---
>  gdb/cli/cli-script.c | 107 +++++++++++++++++++++++++++++++++++++------
>  gdb/top.c            |  10 ++++
>  2 files changed, 103 insertions(+), 14 deletions(-)
> 
> diff --git a/gdb/cli/cli-script.c b/gdb/cli/cli-script.c
> index 4fc9c70259..c70c0e4fb3 100644
> --- a/gdb/cli/cli-script.c
> +++ b/gdb/cli/cli-script.c
> @@ -29,6 +29,7 @@
>  #include "cli/cli-cmds.h"
>  #include "cli/cli-decode.h"
>  #include "cli/cli-script.h"
> +#include "cli/cli-style.h"
>  
>  #include "extension.h"
>  #include "interps.h"
> @@ -1444,10 +1445,22 @@ do_define_command (const char *comname, int from_tty,
>    else
>      cmds = *commands;
>  
> -  newc = add_cmd (comname, class_user, user_defined_command,
> -		  (c && c->theclass == class_user)
> -		  ? c->doc : xstrdup ("User-defined."), list);
> -  newc->user_commands = std::move (cmds);
> +  {
> +    struct cmd_list_element **c_prefixlist = c ? c->prefixlist : nullptr;
> +    const char *c_prefixname = c ? c->prefixname : nullptr;
> +
> +    newc = add_cmd (comname, class_user, user_defined_command,
> +		    (c && c->theclass == class_user)
> +		    ? c->doc : xstrdup ("User-defined."), list);
> +    newc->user_commands = std::move (cmds);
> +
> +    if (c_prefixlist != nullptr)
> +      {
> +	newc->prefixlist = c_prefixlist;
> +	newc->prefixname = c_prefixname;
> +	newc->allow_unknown = newc->user_commands.get () != nullptr;
> +    }
> +  }
I know that it wasn't done right before, but could you please use
c != nullptr in the lines above (and throughout the patch actually)?

Can you also add a bit of comments above to explain the intent of the code?  I
don't really understand what happens there.  I guess it's for when we re-define
a prefix command (do "define foo" after having done "prefix-define foo"), but
it would be nice if we did not have to guess.

>    /* If this new command is a hook, then mark both commands as being
>       tied.  */
> @@ -1522,6 +1535,52 @@ document_command (const char *comname, int from_tty)
>      c->doc = doc;
>    }
>  }
> +
> +/* Implementation of the "prefix-define" command.  */
> +
> +static void
> +prefix_define_command (const char *comname, int from_tty)
> +{
> +  struct cmd_list_element *c, **list;
> +  const char *tem;
> +  const char *comfull;
> +  char *prefixname;
> +
> +  comfull = comname;
> +  list = validate_comname (&comname);
> +
> +  /* Look it up, and verify that we got an exact match.  */
> +  tem = comname;
> +  c = lookup_cmd (&tem, *list, "", -1, 1);
> +  if (c && strcmp (comname, c->name) != 0)
> +    c = nullptr;
> +
> +  if (c && c->theclass != class_user)
> +    error (_("Command \"%s\" is built-in."), comfull);
> +
> +  if (c && c->prefixlist != nullptr)
> +    {
> +      /* c is already a user defined prefix command.  */
> +      return;
> +    }
> +
> +  if (c == nullptr)
> +    {
> +      comname = xstrdup (comname);
> +      c = add_cmd (comname, class_user, user_defined_command,
> +		   xstrdup ("User-defined."), list);

Hmm, the memory ownership management for the command names looks quite complicated.

> +    }

Around here too, I'd appreciate a few comments.  It doesn't have to
be super verbose, but just enough to understand the intent, e.g.:

  /* If the command does not exist at all, create it. */

and below:

  /* Allocate the c->prefixlist, which marks the command as a prefix command. */

> +
> +  c->prefixlist = new struct cmd_list_element*;
> +  *(c->prefixlist) = nullptr;
> +  prefixname = (char *) xmalloc (strlen (comfull) + 2);
> +  prefixname[0] = 0;
> +  strcat (prefixname, comfull);
> +  strcat (prefixname, " ");
> +  c->prefixname = prefixname;

Can that be simplified using xstrprintf:

  c->prefixname = xstrprinf("%s ", comfull);

?

> +  c->allow_unknown = c->user_commands.get () != nullptr;

I'd also like a comment here (and in do_define_command also, where we
also set allow_unknown), to explain why we set allow_unknown to that
particular value.  I guess it's because for user-defined prefix commands,
we don't want to allow passing unknown subcommands, but again it would
be nice to record your intent as an author of the code.

> +}
> +
>  

>  /* Used to implement source_command.  */
>  
> @@ -1562,7 +1621,21 @@ void
>  show_user_1 (struct cmd_list_element *c, const char *prefix, const char *name,
>  	     struct ui_file *stream)
>  {
> -  struct command_line *cmdlines;
> +  if (cli_user_command_p (c))
> +    {
> +      struct command_line *cmdlines = c->user_commands.get ();
> +
> +      fprintf_filtered (stream, "User %scommand \"",
> +			c->prefixlist == NULL ? "" : "prefix ");
> +      fprintf_styled (stream, title_style.style (), "%s%s",
> +		      prefix, name);
> +      fprintf_filtered (stream, "\":\n");
> +      if (cmdlines)
> +	{
> +	  print_command_lines (current_uiout, cmdlines, 1);
> +	  fputs_filtered ("\n", stream);
> +	}
> +    }
>  
>    if (c->prefixlist != NULL)
>      {
> @@ -1571,25 +1644,23 @@ show_user_1 (struct cmd_list_element *c, const char *prefix, const char *name,
>        for (c = *c->prefixlist; c != NULL; c = c->next)
>  	if (c->theclass == class_user || c->prefixlist != NULL)
>  	  show_user_1 (c, prefixname, c->name, gdb_stdout);
> -      return;
>      }
>  
> -  cmdlines = c->user_commands.get ();
> -  fprintf_filtered (stream, "User command \"%s%s\":\n", prefix, name);
> -
> -  if (!cmdlines)
> -    return;
> -  print_command_lines (current_uiout, cmdlines, 1);
> -  fputs_filtered ("\n", stream);
>  }
>  
>  void
>  _initialize_cli_script (void)
>  {
> -  add_com ("document", class_support, document_command, _("\
> +  struct cmd_list_element *c;
> +
> +  /* "document", "define" and "prefix-define" use command_completer,
> +     as this helps the user to either type the command name and/or
> +     its prefixes.  */
> +  c = add_com ("document", class_support, document_command, _("\
>  Document a user-defined command.\n\
>  Give command name as argument.  Give documentation on following lines.\n\
>  End with a line of just \"end\"."));
> +  set_cmd_completer (c, command_completer);
>    define_cmd_element = add_com ("define", class_support, define_command, _("\
>  Define a new command name.  Command name is argument.\n\
>  Definition appears on following lines, one command per line.\n\
> @@ -1598,6 +1669,14 @@ Use the \"document\" command to give documentation for the new command.\n\
>  Commands defined in this way may accept an unlimited number of arguments\n\
>  accessed via $arg0 .. $argN.  $argc tells how many arguments have\n\
>  been passed."));
> +  set_cmd_completer (define_cmd_element, command_completer);
> +  c = add_com ("prefix-define", class_support, prefix_define_command,
> +	   _("\
> +Define or mark a command as a user-defined prefix command.\n\
> +User defined prefix commands can be used as prefix commands for\n\
> +other user defined commands.\n\
> +If the command already exists, it is changed to a prefix command."));
> +  set_cmd_completer (c, command_completer);
>  
>    while_cmd_element = add_com ("while", class_support, while_command, _("\
>  Execute nested commands WHILE the conditional expression is non zero.\n\
> diff --git a/gdb/top.c b/gdb/top.c
> index a1a08e0b99..abd07bd6fe 100644
> --- a/gdb/top.c
> +++ b/gdb/top.c
> @@ -615,6 +615,16 @@ execute_command (const char *p, int from_tty)
>        /* c->user_commands would be NULL in the case of a python command.  */
>        if (c->theclass == class_user && c->user_commands)
>  	execute_user_command (c, arg);
> +      else if (c->theclass == class_user
> +	       && c->prefixlist && !c->allow_unknown)
> +	/* If this is a user defined prefix that does not allow unknown,
> +	   report the list of subcommands.  */

How does one create a user-defined prefix that allows unknown subcommands?

> +	{
> +	  printf_unfiltered
> +	    ("\"%.*s\" must be followed by the name of an %s command.\n",

The "an" preceding the "%s" will sound weird when the prefix is a word that
starts with a consonant:

(gdb) prefix-define banana
(gdb) banana
"banana" must be followed by the name of an banana command.

You could instead just say:

  "banana" must be followed by the name a sub-command.

Simon