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

Pedro Alves
2017-11-27T17:11:49Z

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