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

[Keith Seitz <keiths@redhat.com>]

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