Re: [PATCH 18/40] A smarter linespec completer

[Keith Seitz <keiths at redhat dot com>]

This is awesome. Just some minor nits.

On 06/02/2017 05:22 AM, Pedro Alves wrote:
> gdb/testsuite/ChangeLog:
> yyyy-mm-dd  Pedro Alves  <palves@redhat.com>
> 
>         * gdb.linespec/ls-errs.exp: Don't sent tab characters, now that
>         the completer works.

Typo "sent"

> diff --git a/gdb/completer.h b/gdb/completer.h
> index eab9c69..fbfe4d5 100644
> --- a/gdb/completer.h
> +++ b/gdb/completer.h
> @@ -255,6 +265,14 @@ private:
>       completable words.  */
>    int m_custom_word_point = 0;
>  
> +  /* If true, tell readline to skip appending a whitespace after the
> +     completion.  Automatically set if we have a unique completion
> +     that already has a space at the end.  Completer may also
> +     explicitly set this.  E.g., the linespec completer sets when when

Typos: "... completer sets [this] when *when* .."

> +     the completion ends with the ":" separator between filename and
> +     function name.  */
> +  bool m_suppress_append_ws = false;
> +
>    /* Our idea of lowest common denominator to hand over to readline.
>       See intro.  */
>    char *m_lowest_common_denominator = NULL;
> @@ -347,6 +365,11 @@ extern completer_handle_brkchars_ftype *
>  
>  /* Exported to linespec.c */
>  
> +extern completion_list complete_source_filenames (const char *text);
> +
> +extern void complete_expression (completion_tracker &tracker,
> +				 const char *text, const char *word);
> +
>  extern const char *skip_quoted_chars (const char *, const char *,
>  				      const char *);

Should the explanatory comments in completer.c be moved here?

> diff --git a/gdb/linespec.c b/gdb/linespec.c
> index f24cca2..c993c67 100644
> --- a/gdb/linespec.c
> +++ b/gdb/linespec.c
> @@ -271,6 +293,29 @@ struct ls_parser
>    /* The result of the parse.  */
>    struct linespec result;
>  #define PARSER_RESULT(PPTR) (&(PPTR)->result)
> +
> +  /* What the parser believes the current word point should complete
> +     to.  */
> +  linespec_complete_what complete_what;
> +
> +  /* The completion word point.  The parser advances this as is skips

Typo "as i[t] skips"

> +     tokens.  At some point the input string will end or parsing will
> +     fail, and then we attempt completion at the captured completion
> +     word point, interpreting the string at completion_word as
> +     COMPLETE_WHAT.  */
> +  const char *completion_word;
> +
> +  /* If the current token was a quoted string, then this is the
> +     quoting character (either " or '.).  */

Typo {either " or '.).} (extra period inside parenthesis)

> +  int completion_quote_char;

Why int?

> @@ -543,6 +588,30 @@ find_parameter_list_end (const char *input)
>    return p;
>  }
>  
> +/* If the [STRING, STRING_LEN) string ends with what looks like a
> +   keyword, return the keyword start offset in STRING.  Return -1
> +   otherwise.  */
> +
> +size_t
> +string_find_incomplete_keyword_at_end (const char * const *keywords,
> +				       const char *string, size_t string_len)

Should this be static?

> +  else if (component == linespec_complete_what::FUNCTION)
> +    {
> +      completion_list fn_list;
> +
> +      linespec_complete_function (tracker, text, source_filename);
> +      if (source_filename == NULL)
> +	fn_list = complete_source_filenames (text);
> +

Maybe I took the description of FUNCTION too literally, but it was not obvious to me why we search for source files here.

When parse_linespec returns (after, e.g., "break foo"), PARSER_EXPLICIT->function_name == "foo", but here we search for both functions *and* source files starting with "foo". Given the ambiguity in the grammar, this is a necessary evil. The meaning of FUNCTION is overloaded (it is not just "functions/methods").

Can a comment be added to clarify this (probably at linespec_complete_what::FUNCTION)?

> +  else if (parser.complete_what == linespec_complete_what::FUNCTION)
> +    {
> +      /* While parsing/lexing, we didn't know whether the completion
> +	 word completes to a unique function name already or not.  E.g.:

Similarly here, I would like to see "unique function or source file name". A casual reader may really easily overlook this.

> +	   "b function() <tab>"
> +	 may need to complete either to:
> +	   "b function() const"
> +	 or to:
> +	   "b function() if/thread/task"
> +
> +	 Or, this:
> +	   "b foo t"
> +	 may need to complete either to:
> +	   "b foo template_fun<T>()"
> +	 with "foo" being the template function's return type, or to:
> +	   "b foo thread/task"
> +
> +	 Address that by completing assuming function, and seeing if
> +	 we find a function completion that matches exactly
> +	 "function()". If so, then we advance the completion word past
> +	 the function and switch to KEYWORD completion mode.  */
> +
> +      const char *text = parser.completion_word;
> +      const char *word = parser.completion_word;
> +
> +      complete_linespec_component (&parser, tracker,
> +				   parser.completion_word,
> +				   linespec_complete_what::FUNCTION,
> +				   PARSER_EXPLICIT (&parser)->source_filename);
> +
> +      parser.complete_what = linespec_complete_what::NOTHING;
> +
> +      if (tracker.quote_char ())
> +	{
> +	  /* The function/file name was not close-quoted, so this
> +	     can't be a keyword.  */

quote_char could be ':' (from complete_linespec_component). Please add a comment reminding the reader. [Well, that could easily be me, and I'm gettin' old!]

Keith