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

Keith Seitz
2017-08-08T21:28:00Z

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

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

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

> + LOOKUP_NAME. E.g., in C++, in linespec/wild mode, if the symbol is
> + "foo::function()" and LOOKUP_NAME is "function(", MATCH_FOR_LCD
> + points to "function()" inside SYMBOL_SEARCH_NAME. */
> typedef bool (symbol_name_matcher_ftype)
> (const char *symbol_search_name,
> const lookup_name_info &lookup_name,
> - completion_match *match);
> + completion_match_result *comp_match_res);
>
> /* Some of the structures in this file are space critical.
> The space-critical structures are:
>

Keith