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

Keith Seitz
2017-07-15T00:07:35Z

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