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

[Pedro Alves <palves@redhat.com>]

Hi Keith!

On 07/15/2017 01:07 AM, Keith Seitz wrote:
> 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"

Fixed.

> 
>> 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* .."

Thanks, fixed.

> 
>> +     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?

Moved.

> 
>> 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"

Fixed.

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

Fixed.

> 
>> +  int completion_quote_char;
> 
> Why int?

Mainly because the related readline code uses int too.
E.g., gdb_rl_find_completion_word -> completion_find_completion_word,
and then the tracker uses int for the quote char too.
I suspect that the reason readline uses int it to avoid issues
with having to remember to cast a char to unsigned char when calling
functions like isdigit, etc. everywhere.  We have some functions
in GDB that take a quote char and use int too:

extern void generic_emit_char (int c, struct type *type, struct ui_file *stream,
			       int quoter, const char *encoding);

extern void generic_printstr (struct ui_file *stream, struct type *type, 
			      const gdb_byte *string, unsigned int length, 
			      const char *encoding, int force_ellipses,
			      int quote_char, int c_style_terminator,
			      const struct value_print_options *options);

might be related, not sure.

I can make this a char, and for consistency try to make completer_tracker
use char too, though I'd prefer to do that as a patch on top of this one
that touches every path in a single go, than update the few related
completer_tracker patches, mainly because gdb_rl_find_completion_word
is already in master.  Would you mind that?

> 
>> @@ -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?

Indeed.

> 
>> +  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)?

Sure thing.

> 
>> +  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!]

Here are the comments that I came up with.  WDYT?

From ac03503acbb0796899d51b42c6ac7d9560794bd9 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Mon, 17 Jul 2017 18:45:57 +0100
Subject: [PATCH] address review

---
 gdb/completer.c |  7 ++-----
 gdb/completer.h |  9 +++++++--
 gdb/linespec.c  | 55 ++++++++++++++++++++++++++++++++++++++++++++-----------
 3 files changed, 53 insertions(+), 18 deletions(-)

diff --git a/gdb/completer.c b/gdb/completer.c
index b23baed..eb80a37 100644
--- a/gdb/completer.c
+++ b/gdb/completer.c
@@ -555,8 +555,7 @@ complete_files_symbols (completion_tracker &tracker,
     }
 }
 
-/* Return a list of all source files whose names begin with matching
-   TEXT.  */
+/* See completer.h.  */
 
 completion_list
 complete_source_filenames (const char *text)
@@ -1000,9 +999,7 @@ add_struct_fields (struct type *type, completion_list &output,
     }
 }
 
-/* Complete on expressions.  Often this means completing on symbol
-   names, but some language parsers also have support for completing
-   field names.  */
+/* See completer.h.  */
 
 void
 complete_expression (completion_tracker &tracker,
diff --git a/gdb/completer.h b/gdb/completer.h
index ba66137..f68c6dc 100644
--- a/gdb/completer.h
+++ b/gdb/completer.h
@@ -267,8 +267,8 @@ private:
 
   /* 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
+     that already has a space at the end.  A completer may also
+     explicitly set this.  E.g., the linespec completer sets this when
      the completion ends with the ":" separator between filename and
      function name.  */
   bool m_suppress_append_ws = false;
@@ -371,8 +371,13 @@ extern completer_handle_brkchars_ftype *
 
 /* Exported to linespec.c */
 
+/* Return a list of all source files whose names begin with matching
+   TEXT.  */
 extern completion_list complete_source_filenames (const char *text);
 
+/* Complete on expressions.  Often this means completing on symbol
+   names, but some language parsers also have support for completing
+   field names.  */
 extern void complete_expression (completion_tracker &tracker,
 				 const char *text, const char *word);
 
diff --git a/gdb/linespec.c b/gdb/linespec.c
index f2da551..136cb65 100644
--- a/gdb/linespec.c
+++ b/gdb/linespec.c
@@ -54,7 +54,14 @@ enum class linespec_complete_what
   /* Nothing, no possible completion.  */
   NOTHING,
 
-  /* A function/method name.  */
+  /* A function/method name.  Due to ambiguity between
+
+       (gdb) b source[TAB]
+       source_file.c
+       source_function
+
+     this can also indicate a source filename, iff we haven't seen a
+     separate source filename component, as in "b source.c:function".  */
   FUNCTION,
 
   /* A label symbol.  E.g., break file.c:function:LABEL.  */
@@ -298,7 +305,7 @@ struct ls_parser
      to.  */
   linespec_complete_what complete_what;
 
-  /* The completion word point.  The parser advances this as is skips
+  /* The completion word point.  The parser advances this as it 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
@@ -306,7 +313,7 @@ struct ls_parser
   const char *completion_word;
 
   /* If the current token was a quoted string, then this is the
-     quoting character (either " or '.).  */
+     quoting character (either " or ').  */
   int completion_quote_char;
 
   /* If the current token was a quoted string, then this points at the
@@ -592,7 +599,7 @@ find_parameter_list_end (const char *input)
    keyword, return the keyword start offset in STRING.  Return -1
    otherwise.  */
 
-size_t
+static size_t
 string_find_incomplete_keyword_at_end (const char * const *keywords,
 				       const char *string, size_t string_len)
 {
@@ -2893,7 +2900,12 @@ complete_linespec_component (linespec_parser *parser,
 
       linespec_complete_function (tracker, text, source_filename);
       if (source_filename == NULL)
-	fn_list = complete_source_filenames (text);
+	{
+	  /* Haven't seen a source component, like in "b
+	     file.c:function[TAB]".  Maybe this wasn't a function, but
+	     a filename instead, like "b file.[TAB]".  */
+	  fn_list = complete_source_filenames (text);
+	}
 
       /* If we only have a single filename completion, append a ':' for
 	 the user, since that's the only thing that can usefully follow
@@ -3066,7 +3078,10 @@ linespec_complete (completion_tracker &tracker, const char *text)
   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.:
+	 word completes to a unique function/source name already or
+	 not.
+
+	 E.g.:
 	   "b function() <tab>"
 	 may need to complete either to:
 	   "b function() const"
@@ -3080,10 +3095,26 @@ linespec_complete (completion_tracker &tracker, const char *text)
 	 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.  */
+	 Or, this:
+	   "b file<TAB>"
+	 may need to complete either to a source file name:
+	   "b file.c"
+	 or this, also a filename, but a unique completion:
+	   "b file.c:"
+	 or to a function name:
+	   "b file_function"
+
+	 Address that by completing assuming source or function, and
+	 seeing if we find a completion that matches exactly the
+	 completion word.  If so, then it must be a function (see note
+	 below) and we advance the completion word to the end of input
+	 and switch to KEYWORD completion mode.
+
+	 Note: if we find a unique completion for a source filename,
+	 then it won't match the completion word, because the LCD will
+	 contain a trailing ':'.  And if we're completing at or after
+	 the ':', then complete_linespec_component won't try to
+	 complete on source filenames.  */
 
       const char *text = parser.completion_word;
       const char *word = parser.completion_word;
@@ -3098,7 +3129,9 @@ linespec_complete (completion_tracker &tracker, const char *text)
       if (tracker.quote_char ())
 	{
 	  /* The function/file name was not close-quoted, so this
-	     can't be a keyword.  */
+	     can't be a keyword.  Note: complete_linespec_component
+	     may have swapped the original quote char for ':' when we
+	     get here, but that still indicates the same.  */
 	}
       else if (!tracker.have_completions ())
 	{
-- 
2.5.5