[RFA,1/4] Implement user defined prefix.

  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.

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

> From: Philippe Waroquiers <philippe.waroquiers@skynet.be>
> Cc: Philippe Waroquiers <philippe.waroquiers@skynet.be>
> Date: Sun, 29 Sep 2019 22:54:24 +0200
> 
> +  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\

I suggest to use "user-defined" consistently, with a hyphen.
  

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