Re: [PATCH 31/40] Handle custom completion match prefix / LCD

[Pedro Alves <palves at redhat dot com>]

On 08/08/2017 10:27 PM, Keith Seitz wrote:
> On 06/02/2017 05:22 AM, Pedro Alves wrote:
> 
>> diff --git a/gdb/completer.h b/gdb/completer.h
>> index df90822..db781ab 100644
>> --- a/gdb/completer.h
>> +++ b/gdb/completer.h
>> @@ -116,12 +116,44 @@ private:
>>    std::string m_storage;
>>  };
>>  
>> +/* The result of a successful completion match, but for least common
>> +   denominator (LCD) computation.  Some completers provide matches
>> +   that don't start with the completion "word".  E.g., completing on
>> +   "b push_ba" on a C++ program usually completes to
>> +   std::vector<...>::push_back, std::string::push_back etc.  In such
>> +   case, the symbol comparison routine will set the LCD match to point
>> +   into the "push_back" substring within the symbol's name string.  */
> 
> [missing newline?]

Fixed.

> 
>> +class completion_match_for_lcd
>> +{
>> +public:
>> +  /* Set the match for LCD.  See m_match's description.  */
>> +  void set_match (const char *match)
>> +  { m_match = match; }
>> +
>> +  /* Get the resulting LCD, after a successful match.  */
>> +  const char *finish ()
>> +  { return m_match; }
>> +
>> +  /* Prepare for another completion matching sequence.  */
>> +  void clear ()
>> +  { m_match = NULL; }
>> +
>> +private:
>> +  /* The completion match result for LCD.  This is usually either a
>> +     pointer into to a substring within a symbol's name, or to the
>> +     storage of the pairing completion_match object.  */
>> +  const char *m_match;
>> +};
>> +
>>  /* Convenience aggregate holding info returned by the symbol name
>>     matching routines (see symbol_name_matcher_ftype).  */
>>  struct completion_match_result
>>  {
>>    /* The completion match candidate.  */
>>    completion_match match;
>> +
>> +  /* The completion match, for LCD computation purposes.  */
>> +  completion_match_for_lcd match_for_lcd;
>>  };
>>  
>>  /* The final result of a completion that is handed over to either
>> diff --git a/gdb/cp-support.c b/gdb/cp-support.c
>> index 064a45b..397c738 100644
>> --- a/gdb/cp-support.c
>> +++ b/gdb/cp-support.c
>> @@ -1660,8 +1660,11 @@ cp_fq_symbol_name_matches (const char *symbol_search_name,
>>  			    name.c_str (), name.size (),
>>  			    mode) == 0)
>>      {
>> -      if (match != NULL)
>> -	match->set_match (symbol_search_name);
>> +      if (comp_match_res != NULL)
>> +	{
>> +	  comp_match_res->match.set_match (symbol_search_name);
>> +	  comp_match_res->match_for_lcd.set_match (symbol_search_name);
>> +	}
>>        return true;
>>      }
>>  
>> diff --git a/gdb/language.c b/gdb/language.c
>> index 6705a3b..511a86f 100644
>> --- a/gdb/language.c
>> +++ b/gdb/language.c
>> @@ -725,8 +725,11 @@ default_symbol_name_matcher (const char *symbol_search_name,
>>    if (strncmp_iw_with_mode (symbol_search_name, name.c_str (), name.size (),
>>  			    mode) == 0)
>>      {
>> -      if (match != NULL)
>> -	match->set_match (symbol_search_name);
>> +      if (comp_match_res != NULL)
>> +	{
>> +	  comp_match_res->match.set_match (symbol_search_name);
>> +	  comp_match_res->match_for_lcd.set_match (symbol_search_name);
>> +	}
>>        return true;
>>      }
>>    else
> 
> In the above two hunks, the code calls result->match.set_match (A)
> and result->match_for_lcd.set_match (A), i.e., both these calls take the
> same argument and are called at the same time.
> 
> Furthermore, on the final series of the patch,
> 
> $ grep -n set_match\ \( *.c
> ada-lang.c:6416:	  comp_match_res->match.set_match (match_str.c_str ());
> ada-lang.c:6417:	  comp_match_res->match_for_lcd.set_match (match_str.c_str ());
> ada-lang.c:6426:	  comp_match_res->match.set_match (match_str.c_str ());
> ada-lang.c:6427:	  comp_match_res->match_for_lcd.set_match (match_str.c_str ());
> cp-support.c:1734:	      comp_match_res->match.set_match (symbol_search_name);
> cp-support.c:1735:	      comp_match_res->match_for_lcd.set_match (sname);
> cp-support.c:1773:	  comp_match_res->match.set_match (symbol_search_name);
> cp-support.c:1774:	  comp_match_res->match_for_lcd.set_match (symbol_search_name);
> language.c:725:	  comp_match_res->match.set_match (symbol_search_name);
> language.c:726:	  comp_match_res->match_for_lcd.set_match (symbol_search_name);
> 
> It appears that these two are /always/ called together, suggesting that
> completion_match_result might "benefit" from a convenience method to set both
> of these parameters at the same time. [Unless there is a specific reason
> not to do this, of course.] WDYT?

OK, I'm folding the below into the series.  (I'm posting a v2 in a bit.)

>From cc3d60557be486a0eb317e6920ccb323a76617c8 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Mon, 27 Nov 2017 12:26:31 +0000
Subject: [PATCH] set_match

---
 gdb/ada-lang.c   | 10 +++-------
 gdb/completer.h  | 11 +++++++++++
 gdb/cp-support.c | 29 +++++++++++++++++++++++------
 gdb/language.c   |  5 +----
 4 files changed, 38 insertions(+), 17 deletions(-)

diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index cad9d7c..38d1ce6 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -6420,11 +6420,7 @@ ada_lookup_name_info::matches
       std::string &match_str = comp_match_res->match.storage ();
 
       if (!m_encoded_p)
-	{
-	  match_str = ada_decode (sym_name);
-	  comp_match_res->match.set_match (match_str.c_str ());
-	  comp_match_res->match_for_lcd.set_match (match_str.c_str ());
-	}
+	match_str = ada_decode (sym_name);
       else
 	{
 	  if (m_verbatim_p)
@@ -6432,9 +6428,9 @@ ada_lookup_name_info::matches
 	  else
 	    match_str = sym_name;
 
-	  comp_match_res->match.set_match (match_str.c_str ());
-	  comp_match_res->match_for_lcd.set_match (match_str.c_str ());
 	}
+
+      comp_match_res->set_match (match_str.c_str ());
     }
 
   return true;
diff --git a/gdb/completer.h b/gdb/completer.h
index 97611e4..f756412 100644
--- a/gdb/completer.h
+++ b/gdb/completer.h
@@ -155,6 +155,17 @@ struct completion_match_result
 
   /* The completion match, for LCD computation purposes.  */
   completion_match_for_lcd match_for_lcd;
+
+  /* Convenience that sets both MATCH and MATCH_FOR_LCD.  M_FOR_LCD is
+     optional.  If not specified, defaults to M.  */
+  void set_match (const char *m, const char *m_for_lcd = NULL)
+  {
+    match.set_match (m);
+    if (m_for_lcd == NULL)
+      match_for_lcd.set_match (m);
+    else
+      match_for_lcd.set_match (m_for_lcd);
+  }
 };
 
 /* The final result of a completion that is handed over to either
diff --git a/gdb/cp-support.c b/gdb/cp-support.c
index 4c78af8..172d821 100644
--- a/gdb/cp-support.c
+++ b/gdb/cp-support.c
@@ -1682,8 +1682,28 @@ cp_symbol_name_matches_1 (const char *symbol_search_name,
 	{
 	  if (comp_match_res != NULL)
 	    {
-	      comp_match_res->match.set_match (symbol_search_name);
-	      comp_match_res->match_for_lcd.set_match (sname);
+	      /* Note here we set different MATCH and MATCH_FOR_LCD
+		 strings.  This is because with
+
+		  (gdb) b push_bac[TAB]
+
+		 we want the completion matches to list
+
+		  std::vector<int>::push_back(...)
+		  std::vector<char>::push_back(...)
+
+		 etc., which are SYMBOL_SEARCH_NAMEs, while we want
+		 the input line to auto-complete to
+
+		  (gdb) push_back(...)
+
+		 which is SNAME, not to
+
+		  (gdb) std::vector<
+
+		 which would be the regular common prefix between all
+		 the matches otherwise.  */
+	      comp_match_res->set_match (symbol_search_name, sname);
 	    }
 	  return true;
 	}
@@ -1718,10 +1738,7 @@ cp_fq_symbol_name_matches (const char *symbol_search_name,
 			    mode, language_cplus) == 0)
     {
       if (comp_match_res != NULL)
-	{
-	  comp_match_res->match.set_match (symbol_search_name);
-	  comp_match_res->match_for_lcd.set_match (symbol_search_name);
-	}
+	comp_match_res->set_match (symbol_search_name);
       return true;
     }
 
diff --git a/gdb/language.c b/gdb/language.c
index 64ef7e0..c05b703 100644
--- a/gdb/language.c
+++ b/gdb/language.c
@@ -716,10 +716,7 @@ default_symbol_name_matcher (const char *symbol_search_name,
 			    mode, language_minimal) == 0)
     {
       if (comp_match_res != NULL)
-	{
-	  comp_match_res->match.set_match (symbol_search_name);
-	  comp_match_res->match_for_lcd.set_match (symbol_search_name);
-	}
+	comp_match_res->set_match (symbol_search_name);
       return true;
     }
   else
-- 
2.5.5


> 
>> diff --git a/gdb/symtab.h b/gdb/symtab.h
>> index d68eed8..736fea0 100644
>> --- a/gdb/symtab.h
>> +++ b/gdb/symtab.h
>> @@ -292,15 +292,21 @@ private:
>>  
>>     SYMBOL_SEARCH_NAME should be a symbol's "search" name.
>>  
>> -   On success and if non-NULL, MATCH is set to point to the symbol
>> -   name as should be presented to the user as a completion match list
>> -   element.  In most languages, this is the same as the symbol's
>> -   search name, but in some, like Ada, the display name is dynamically
>> -   computed within the comparison routine.  */
>> +   On success and if non-NULL, COMP_MATCH_RES->match is set to point
>> +   to the symbol name as should be presented to the user as a
>> +   completion match list element.  In most languages, this is the same
>> +   as the symbol's search name, but in some, like Ada, the display
>> +   name is dynamically computed within the comparison routine.
>> +
>> +   Also, on success and if non-NULL, COMP_MATCH_RES->match_for_lcd
>> +   points the part of SYMBOL_SEARCH_NAME that was considered to match
>             ^
> missing "to"

Fixed, thanks.

-- 
Pedro Alves