Re: [RFA] completion in command definition

[Pedro Alves <palves@redhat.com>]

Hi Jerome,

On 01/12/2017 10:05 AM, Jerome Guitton wrote:

> commit 05bd4353619a49e8506bc6f8cd428b6a0961b28f
> Author: Jerome Guitton <guitton@adacore.com>
> Date:   Tue Jan 10 15:15:53 2017 +0100
> 
>     [RFA] completion in command definition
>     
>     When defining a new macro, "command" is not recognized as an aliased for
>     "commands":
>     
>      (gdb) define breakmain
>      Type commands for definition of "breakmain".
>      End with a line saying just "end".
>      >break main
>      >command
>      >echo "IN MAIN\n"
>      >end
>      (gdb)
>     
>     There is a special case for while-stepping, where 'ws' and 'stepping' are
>     recognized explicitely. We could add "command" to the list as well. I'd
>     rather use cli-decode though. Patch in attachment, with a test, tested
>     in x86-linux. OK to apply?

OK (but please remember to edit out the question and "attachment"
from the commit log).

>     
>     gdb/ChangeLog:
>     	* cli/cli-decode.c (find_command_name_length): Make it global.

s/global/extern/

>     	* cli/cli-decode.h (find_command_name_length): Declare.
>     	* cli/cli-script.c (command_name_equals, line_first_arg):
>     	New functions.
>     	(process_next_line): Use cli-decode to parse command names

Missing period.

>     
>     gdb/testsuite/ChangeLog:
>     
>     	* gdb.base/define.exp: Add test for completion in command definition.
> 
> diff --git a/gdb/cli/cli-decode.c b/gdb/cli/cli-decode.c
> index dbd874e..b71f6f6 100644
> --- a/gdb/cli/cli-decode.c
> +++ b/gdb/cli/cli-decode.c
> @@ -1255,7 +1255,7 @@ find_cmd (const char *command, int len, struct cmd_list_element *clist,
>    return found;
>  }
>  
> -static int
> +int
>  find_command_name_length (const char *text)
>  {
>    const char *p = text;
> diff --git a/gdb/cli/cli-decode.h b/gdb/cli/cli-decode.h
> index e5ab839..66159fd 100644
> --- a/gdb/cli/cli-decode.h
> +++ b/gdb/cli/cli-decode.h
> @@ -253,4 +253,6 @@ extern const char * const auto_boolean_enums[];
>  
>  extern int cli_user_command_p (struct cmd_list_element *);
>  
> +extern int find_command_name_length (const char *);
> +

Now that this is a public function, it'd be nice to
document its interface.

>  #endif /* !defined (CLI_DECODE_H) */
> diff --git a/gdb/cli/cli-script.c b/gdb/cli/cli-script.c
> index 6f44d52..f19df9e 100644
> --- a/gdb/cli/cli-script.c
> +++ b/gdb/cli/cli-script.c
> @@ -143,7 +143,7 @@ multi_line_command_p (enum command_control_type type)
>     control commands (if/while).  */
>  
>  static struct command_line *
> -build_command_line (enum command_control_type type, char *args)
> +build_command_line (enum command_control_type type, const char *args)

Mention this in the ChangeLog.

>  {
>    struct command_line *cmd;
>  
> @@ -904,6 +904,30 @@ read_next_line (void)
>    return command_line_input (prompt_ptr, from_tty, "commands");
>  }
>  
> +/* Return non-zero if CMD's name is NAME.  */
> +
> +static bool
> +command_name_equals (struct cmd_list_element *cmd, const char *name)
> +{
> +  return cmd != NULL
> +    && cmd != CMD_LIST_AMBIGUOUS
> +    && strcmp (cmd->name, name) == 0;

Multi-line conditionals should be wrapped in ()s, and then
the && naturally aligns under "cmd":

  return (cmd != NULL
          && cmd != CMD_LIST_AMBIGUOUS
          && strcmp (cmd->name, name) == 0);

(and suitably tabified).

> +}
> +
> +/* Given an input line P, skip the command and return a pointer to the
> +   first argument.  */
> +
> +static const char *
> +line_first_arg (const char *p)
> +{
> +  const char *first_arg = p + find_command_name_length (p);
> +
> +  while (first_arg != '\0' && isspace (*first_arg))
> +    first_arg++;

   return skip_spaces_const (first_arg);

?


> +
> +  return first_arg;
> +}
> +
>  /* Process one input line.  If the command is an "end", return such an
>     indication to the caller.  If PARSE_COMMANDS is true, strip leading
>     whitespace (trailing whitespace is always stripped) in the line,
> @@ -919,6 +943,8 @@ process_next_line (char *p, struct command_line **command, int parse_commands,
>    char *p_end;
>    char *p_start;
>    int not_handled = 0;
> +  const char *cmd_name = p;
> +  struct cmd_list_element *cmd;

Move these to the scope that needs them?

>  
>    /* Not sure what to do here.  */
>    if (p == NULL)
> @@ -938,9 +964,11 @@ process_next_line (char *p, struct command_line **command, int parse_commands,
>       We also permit whitespace before end and after.  */
>    if (p_end - p_start == 3 && startswith (p_start, "end"))
>      return end_command;
> -  
> +
>    if (parse_commands)
>      {
> +      cmd = lookup_cmd_1 (&cmd_name, cmdlist, NULL, 1);

It'd be good to add a comment about why we do this instead of
just doing a strcmp.  Mention something about aliases and
unique command abbreviations.  Please tweak the subject
line / commit log too; I initially thought this was going
to be about tab completion.  :-)

> +
>        /* If commands are parsed, we skip initial spaces.  Otherwise,
>  	 which is the case for Python commands and documentation
>  	 (see the 'document' command), spaces are preserved.  */
> @@ -958,9 +986,7 @@ process_next_line (char *p, struct command_line **command, int parse_commands,
>  
>        /* Check for while, if, break, continue, etc and build a new
>  	 command line structure for them.  */
> -      if ((p_end - p >= 14 && startswith (p, "while-stepping"))
> -	  || (p_end - p >= 8 && startswith (p, "stepping"))
> -	  || (p_end - p >= 2 && startswith (p, "ws")))
> +      if (command_name_equals (cmd, "while-stepping"))
>  	{
>  	  /* Because validate_actionline and encode_action lookup
>  	     command's line as command, we need the line to
> @@ -975,40 +1001,25 @@ process_next_line (char *p, struct command_line **command, int parse_commands,
>  	     not.  */
>  	  *command = build_command_line (while_stepping_control, p);
>  	}
> -      else if (p_end - p > 5 && startswith (p, "while"))
> +      else if (command_name_equals (cmd, "while"))
>  	{
> -	  char *first_arg;
> -
> -	  first_arg = p + 5;
> -	  while (first_arg < p_end && isspace (*first_arg))
> -	    first_arg++;
> -	  *command = build_command_line (while_control, first_arg);
> +	  *command = build_command_line (while_control, line_first_arg (p));
>  	}
> -      else if (p_end - p > 2 && startswith (p, "if"))
> +      else if (command_name_equals (cmd, "if"))
>  	{
> -	  char *first_arg;
> -
> -	  first_arg = p + 2;
> -	  while (first_arg < p_end && isspace (*first_arg))
> -	    first_arg++;
> -	  *command = build_command_line (if_control, first_arg);
> +	  *command = build_command_line (if_control, line_first_arg (p));
>  	}
> -      else if (p_end - p >= 8 && startswith (p, "commands"))
> +      else if (command_name_equals (cmd, "commands"))
>  	{
> -	  char *first_arg;
> -
> -	  first_arg = p + 8;
> -	  while (first_arg < p_end && isspace (*first_arg))
> -	    first_arg++;
> -	  *command = build_command_line (commands_control, first_arg);
> +	  *command = build_command_line (commands_control, line_first_arg (p));
>  	}
> -      else if (p_end - p == 6 && startswith (p, "python"))
> +      else if (command_name_equals (cmd, "python"))
>  	{
>  	  /* Note that we ignore the inline "python command" form
>  	     here.  */
>  	  *command = build_command_line (python_control, "");
>  	}
> -      else if (p_end - p == 6 && startswith (p, "compile"))
> +      else if (command_name_equals (cmd, "compile"))
>  	{
>  	  /* Note that we ignore the inline "compile command" form
>  	     here.  */
> @@ -1016,7 +1027,7 @@ process_next_line (char *p, struct command_line **command, int parse_commands,
>  	  (*command)->control_u.compile.scope = COMPILE_I_INVALID_SCOPE;
>  	}
>  
> -      else if (p_end - p == 5 && startswith (p, "guile"))
> +      else if (command_name_equals (cmd, "guile"))
>  	{
>  	  /* Note that we ignore the inline "guile command" form here.  */
>  	  *command = build_command_line (guile_control, "");
> diff --git a/gdb/testsuite/gdb.base/define.exp b/gdb/testsuite/gdb.base/define.exp
> index 59203ec..b57b068 100644
> --- a/gdb/testsuite/gdb.base/define.exp
> +++ b/gdb/testsuite/gdb.base/define.exp
> @@ -147,6 +147,30 @@ gdb_test_multiple "define ifnospace" "define user command: ifnospace" \
>  
>  gdb_test "ifnospace" ".*hi there.*" "test ifnospace is parsed correctly"
>  
> +# Verify that the command parser properly handle completion.

Here too.

> +set test "define use command: breakmain"

typo: "user" ?

> +gdb_test_multiple "define breakmain" "$test" \
> +{
> +  -re "Type commands for definition of \"breakmain\".\r\nEnd with a line saying just \"end\".\r\n>$" \
> +    {
> +      pass "$test"
> +
> +      set test "send body of breakmain"
> +      gdb_test_multiple "break main\ncommand\necho\nend\nend" "$test"  \
> +        {
> +         -re "$gdb_prompt $"\
> +                 {pass "$test"}

Does this fail with an unfixed GDB?  I ask since this is just matching the
prompt, which a "bad" GDB also outputs?

> +        }
> +    }
> +}
> +
> +gdb_test "breakmain" ".*Breakpoint 2.*" "test command completion in define"

Odd message for this test.  I suggest wrapping these three
tests in

with_test_prefix "command abbreviations in define" {
  set test "define user command: breakmain"
  gdb_test_multiple "define breakmain" "$test" .....

  gdb_test "breakmain" ".*Breakpoint 2.*" "run user command"

  gdb_test "info break 2" \ 
    .... \
    "info break shows echo command"
}

> +
> +gdb_test "info break 2" \
> +    "Num     Type\[ \]+Disp Enb Address\[ \]+What.*
> +\[0-9\]+\[\t \]+breakpoint     keep y.* in main at .*
> +\[\t \]+echo.*"
> +

It's better to avoid hardcoding breakpoint numbers, since
it'll break if someone adds some test before this one
that creates some other breakpoint.

You can use "$bpnum" instead to avoid that.


>  # Verify that the command parser doesn't require a space after an 'while'
>  # command in a user defined function.
>  #
> 

Thanks,
Pedro Alves