[3/6] gdb: simplify completion_result::print_matches

  Simplify completion_result::print_matches by removing one of the code
paths.  Now, every time we call ::print_matches we always add the
trailing quote.

Previously, when using the 'complete' command, if there was only one
result then trailing quote was added in ::build_completion_result, but
when we had multiple results the trailing quote was added in
::print_matches.  As a consequence, ::print_matches had to understand
not to add the trailing quote for the single result case.

After this commit we don't add the trailing quote in
::build_completion_result, instead ::print_matches always adds the
trailing quote, which makes ::print_matches simpler.

However, there is a slight problem.  When completion is being driven
by readline, and not by the 'complete' command, we still need to
manually add the trailing quote in the single result case, and as the
printing is done by readline we can't add the quote at the time of
printing, and so, in ::build_completion_result, we still add the
trailing quote, but only when completion is being done for readline.

And this does cause a small problem.  When completing a filename, if
the completion results in a directory name then, when using the
'complete' command, GDB should not be adding a trailing quote.  For
example, if we have the file /tmp/xxx/foo.c, then what we should see
is this:

  (gdb) complete file '/tmp/xx
  file 'tmp/xxx/

But what we actually see after this commit is this:

  (gdb) complete file '/tmp/xx
  file 'tmp/xxx/'

Previously we didn't get the trailing quote in this case, as when
there is only a single result, the quote was added in
::build_completion_result, and for filename completion, GDB didn't
know what the quote character was in ::build_completion_result, so no
quote was added.  Now that the trailing quote is always added in
::print_matches, and GDB does know the quote character at this point,
so we are now getting the trailing quote, which is not correct.

This is a regression, but really, GDB is now broken in a consistent
way, if we create the file /tmp/xxa/bar.c, then previously if we did
this:

  (gdb) complete file '/tmp/xx
  file '/tmp/xxa/'
  file '/tmp/xxx/'

Notice how we get the trailing quote in this case, this is the before
patch behaviour, and is also wrong.

A later commit will fix things so that the trailing quote is not added
in this filename completion case, but for now I'm going to accept this
small regression.

This change in behaviour caused some failures in one of the completion
tests, I've tweaked the test case to expect the trailing quote as part
of this commit, but will revert this in a later commit in this series.

I've also added an extra test for when the 'complete' command does
complete to a single complete filename, in which case the trailing
quote is expected.
---
 gdb/completer.c                               | 62 +++++++++----------
 .../gdb.base/filename-completion.exp          | 17 ++++-
 2 files changed, 46 insertions(+), 33 deletions(-)
  

Hi Andlew,

I have a couple of minor comment below.

> diff --git a/gdb/completer.c b/gdb/completer.c
> index 9b4041da01a..2b3972213d8 100644
> --- a/gdb/completer.c
> +++ b/gdb/completer.c
> @@ -2311,23 +2311,30 @@ completion_tracker::build_completion_result (const char *text,
>  
>    if (m_lowest_common_denominator_unique)
>      {
> -      /* We don't rely on readline appending the quote char as
> -	 delimiter as then readline wouldn't append the ' ' after the
> -	 completion.  */
> -      char buf[2] = { (char) quote_char () };
> -
> -      match_list[0] = reconcat (match_list[0], match_list[0],
> -				buf, (char *) NULL);
> -      match_list[1] = NULL;
> -
> -      /* If the tracker wants to, or we already have a space at the
> -	 end of the match, tell readline to skip appending
> -	 another.  */
> -      char *match = match_list[0];
> -      bool completion_suppress_append
> -	= (suppress_append_ws ()
> -	   || (match[0] != '\0'
> -	       && match[strlen (match) - 1] == ' '));
> +      bool completion_suppress_append;
> +
> +      if (from_readline ())
> +	{
> +	  /* We don't rely on readline appending the quote char as
> +	     delimiter as then readline wouldn't append the ' ' after the
> +	     completion.  */
> +	  char buf[2] = { (char) quote_char () };

To be consistent with what is used below, "{ (char) quote_char (), '\0' }"

> +
> +	  match_list[0] = reconcat (match_list[0], match_list[0], buf,
> +				    (char *) nullptr);
> +
> +	  /* If the tracker wants to, or we already have a space at the end
> +	     of the match, tell readline to skip appending another.  */
> +	  char *match = match_list[0];
> +	  completion_suppress_append
> +	    = (suppress_append_ws ()
> +	       || (match[0] != '\0'
> +		   && match[strlen (match) - 1] == ' '));
> +	}
> +      else
> +	completion_suppress_append = false;
> +
> +      match_list[1] = nullptr;
>  
>        return completion_result (match_list, 1, completion_suppress_append);
>      }
> @@ -2449,21 +2456,14 @@ void
>  completion_result::print_matches (const std::string &prefix,
>  				  const char *word, int quote_char)
>  {
> -  if (this->number_matches == 1)
> -    printf_unfiltered ("%s%s\n", prefix.c_str (), this->match_list[0]);
> -  else
> -    {
> -      this->sort_match_list ();
> +  this->sort_match_list ();
>  
> -      for (size_t i = 0; i < this->number_matches; i++)
> -	{
> -	  printf_unfiltered ("%s%s", prefix.c_str (),
> -			     this->match_list[i + 1]);
> -	  if (quote_char)
> -	    printf_unfiltered ("%c", quote_char);
> -	  printf_unfiltered ("\n");
> -	}
> -    }
> +  char buf[2] = { (char) quote_char, '\0' };
> +  size_t off = this->number_matches == 1 ? 0 : 1;
> +
> +  for (size_t i = 0; i < this->number_matches; i++)
> +    printf_unfiltered ("%s%s%s\n", prefix.c_str (),
> +		       this->match_list[i + off], buf);
>  
>    if (this->number_matches == max_completions)
>      {
> diff --git a/gdb/testsuite/gdb.base/filename-completion.exp b/gdb/testsuite/gdb.base/filename-completion.exp
> index f23e8671f40..66e5f411795 100644
> --- a/gdb/testsuite/gdb.base/filename-completion.exp
> +++ b/gdb/testsuite/gdb.base/filename-completion.exp
> @@ -63,8 +63,21 @@ proc run_tests { root } {
>  	test_gdb_complete_none "file ${qc}${root}/xx" \
>  	    "expand a non-existent filename"
>  
> -	test_gdb_complete_unique "file ${qc}${root}/a" \
> -	    "file ${qc}${root}/aaa/" "" false \
> +	# The following test is split into separate cmd and tab calls as the
> +	# cmd versions will add a closing quote.  It shouldn't be doing
> +	# this; this will be fixed in a later commit.

Instead of testing the "wrong" behavior, if you intend to fix that in a
later patch, I’d be tempted to prefer to just use "setup_kfail".

> +	test_gdb_complete_cmd_unique "file ${qc}${root}/a" \
> +	    "file ${qc}${root}/aaa/${qc}" \
> +	    "expand a unique directory name"
> +
> +	if { [readline_is_used] } {
> +	    test_gdb_complete_tab_unique "file ${qc}${root}/a" \
> +		"file ${qc}${root}/aaa/" "" \
> +		"expand a unique directory name"
> +	}
> +
> +	test_gdb_complete_unique "file ${qc}${root}/cc2" \
> +	    "file ${qc}${root}/cc2${qc}" " " false \
>  	    "expand a unique filename"
>  
>  	test_gdb_complete_multiple "file ${qc}${root}/" \
> -- 
> 2.25.4
> 

Best,
Lancelot.