Re: [RFA] completion in command definition

[Simon Marchi <simon.marchi@polymtl.ca>]

Hi Jerome,

I can comment on styling issues (some of them nits), but not so much on 
the actual problem, since I'm not very familiar with it.  But in general 
it cleans up the code nicely I think.

> diff --git a/gdb/cli/cli-decode.h b/gdb/cli/cli-decode.h
> index e5ab839..83bc34a 100644
> --- a/gdb/cli/cli-decode.h
> +++ b/gdb/cli/cli-decode.h
> @@ -253,4 +253,5 @@ 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 *);

Add new line here?

>  #endif /* !defined (CLI_DECODE_H) */
> diff --git a/gdb/cli/cli-script.c b/gdb/cli/cli-script.c
> index 6f44d52..338d726 100644
> --- a/gdb/cli/cli-script.c
> +++ b/gdb/cli/cli-script.c
> @@ -904,6 +904,28 @@ read_next_line (void)
>    return command_line_input (prompt_ptr, from_tty, "commands");
>  }
> 
> +/* Return non-zero if CMD's name is NAME.  */
> +
> +static int
> +command_name_equals (struct cmd_list_element *cmd, char *name)

This can return a bool, and the command can say true instead of 
non-zero.

name can be "const char *".

> +{
> +  return cmd

For null pointer checks, use "cmd != NULL" or "cmd != nullptr".

> +    && cmd != CMD_LIST_AMBIGUOUS
> +    && strcmp (cmd->name, name) == 0;
> +}
> +
> +/* Given an input line P, skip the command and return a pointer to the
> +   first argument.  */
> +
> +static char *
> +line_first_arg (char *p)

The argument type and return type can be const.  It will require you to 
constify a few other things which the compiler will tell you.

> +{
> +  char *first_arg = p + find_command_name_length (p);

Add an empty line here...

> +  while (first_arg != '\0' && isspace (*first_arg))
> +    first_arg++;

... and here.

> +  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 +941,9 @@ process_next_line (char *p, struct command_line
> **command, int parse_commands,
>    char *p_end;
>    char *p_start;
>    int not_handled = 0;
> +  char *cmd_name = p;

This can be constified.

> +  struct cmd_list_element *last_line = 0;

This should be either "= NULL" or "= nullptr", but actually I don't 
think it needs to be initialized.

> +  struct cmd_list_element *cmd;
> 
>    /* Not sure what to do here.  */
>    if (p == NULL)
> @@ -938,9 +963,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 ((const char **) &cmd_name, cmdlist, 
> &last_line, 1);

If cmd_name is constified, the const char cast can be removed.

> diff --git a/gdb/testsuite/gdb.base/define.exp
> b/gdb/testsuite/gdb.base/define.exp
> index 59203ec..355a7bc 100644
> --- a/gdb/testsuite/gdb.base/define.exp
> +++ b/gdb/testsuite/gdb.base/define.exp
> @@ -147,6 +147,27 @@ 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.
> +
> +gdb_test_multiple "define breakmain" "define user command: breakmain" 
> \
> +{
> +  -re "Type commands for definition of \"breakmain\".\r\nEnd with a
> line saying just \"end\".\r\n>$" \
> +    {
> +      gdb_test_multiple "break main\ncommand\necho\nend\nend" "send
> body of breakmain"  \
> +        {
> +         -re "$gdb_prompt $"\
> +                 {pass "define user command: breakmain"}
> +        }
> +    }
> +}

I think it's preferable to use the same test name in the 
gdb_test_multiple and in the pass.  Usually, we define a variable with 
the test name and use it for both.  Also, if there are two 
gdb_test_multiple, I guess we would need two "pass"? Like:

set test "define user command: breakmain"
gdb_test_multiple ... $test {
   -re ... {
     pass $test

     set test "send body of breakmain"
     gdb_test_multiple ... $test {
       -re ... {
         pass $test
       }
     }
   }
}

I'm not sure about that...

Thanks!

Simon