Re: [PATCH 25/40] Introduce lookup_name_info and generalize Ada's FULL/WILD name matching

[Keith Seitz <keiths at redhat dot com>]

On 07/20/2017 12:06 PM, Pedro Alves wrote:
> On 07/20/2017 08:00 PM, Pedro Alves wrote:
> 
>> I'll send the updated patch #25 as a reply.
> 
> Here's the updated patch.  I've also force-pushed this to
> the users/palves/cxx-breakpoint-improvements branch, meanwhile
> rebased to current master.

Awesome. I've pulled your changes to retest, and I found no regressions
this time. :-)

> 	* ada-lang.c (ada_encode): Rename to ..
> 	(ada_encode_1): ... this.  Add throw_errors parameter and handle
> 	it.
> 	(ada_encode): Reimplement.
> 	(match_name): Delete, folded into full_name.
> 	(ada_lookup_simple_minsym): Use lookup_name_info and the
> 	language's symbol_name_matcher_ftype.
> 	(add_symbols_from_enclosing_procs, ada_add_local_symbols)
> 	(ada_add_block_symbols, ada_add_block_symbols): Adjust to use
> 	lookup_name_info.

ada_add_block_symbols is listed twice.

> 	* linespec.h (linespec_complete_function)
> 	(linespec_complete_label): Add name_match_type parameter.

I don't actually see any linespec.h diffs...

> 	* psymtab.c (psym_lookup_symbol): Use lookup_name_info.
> 	(match_partial_symbol): Use symbol_name_match_type and
> 	lookup_name_info.

Mention psymbol_name_matches (since you've mentioned it in other entries).

> 	* symfile-debug.c (debug_qf_map_matching_symbols)
> 	(debug_qf_map_matching_symbols): Use symbol_name_match_type.
> 	(debug_qf_expand_symtabs_matching): use lookup_name_info.
                                            ^
typo (lowercase letter)

> diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
> index b6eb382..58d51d7 100644
> --- a/gdb/ada-lang.c
> +++ b/gdb/ada-lang.c
> @@ -4935,23 +4928,19 @@ ada_lookup_simple_minsym (const char *name)
>    struct bound_minimal_symbol result;
>    struct objfile *objfile;
>    struct minimal_symbol *msymbol;
> -  const int wild_match_p = should_use_wild_match (name);
>  
>    memset (&result, 0, sizeof (result));
>  
> -  /* Special case: If the user specifies a symbol name inside package
> -     Standard, do a non-wild matching of the symbol name without
> -     the "standard__" prefix.  This was primarily introduced in order
> -     to allow the user to specifically access the standard exceptions
> -     using, for instance, Standard.Constraint_Error when Constraint_Error
> -     is ambiguous (due to the user defining its own Constraint_Error
> -     entity inside its program).  */
> -  if (startswith (name, "standard__"))
> -    name += sizeof ("standard__") - 1;
> +  symbol_name_match_type match_type = name_match_type_from_name (name);
> +  lookup_name_info lookup_name (name, match_type);
> +
> +  symbol_name_matcher_ftype *match_name
> +    = language_get_symbol_name_matcher (language_def (language_ada),
> +					lookup_name);

Okay, I understand that you did this to be consistent with generic code, but
it seems odd to use all that machinery to access ada_get_symbol_name_mathcer,
which is defined in this file. Just sayin' it kinda surprised me.

>  
>    ALL_MSYMBOLS (objfile, msymbol)
>    {
> -    if (match_name (MSYMBOL_LINKAGE_NAME (msymbol), name, wild_match_p)
> +    if (match_name (MSYMBOL_LINKAGE_NAME (msymbol), lookup_name, NULL)
>          && MSYMBOL_TYPE (msymbol) != mst_solib_trampoline)
>        {
>  	result.minsym = msymbol;
> @@ -5501,28 +5489,28 @@ aux_add_nonlocal_symbols (struct block *block, struct symbol *sym, void *data0)
>    return 0;
>  }
>  
> -/* Helper for add_nonlocal_symbols.  Find symbols in DOMAIN which are targetted
> -   by renamings matching NAME in BLOCK.  Add these symbols to OBSTACKP.  If
> -   WILD_MATCH_P is nonzero, perform the naming matching in "wild" mode (see
> -   function "wild_match" for more information).  Return whether we found such
> -   symbols.  */
> +/* Helper for add_nonlocal_symbols.  Find symbols in DOMAIN which are
> +   targetted by renamings matching LOOKUP_NAME in BLOCK.  Add these

*targeted

> +   symbols to OBSTACKP.  Return whether we found such symbols.  */
>  
>  static int
>  ada_add_block_renamings (struct obstack *obstackp,
>  			 const struct block *block,
> -			 const char *name,
> -			 domain_enum domain,
> -			 int wild_match_p)
> +			 const lookup_name_info &lookup_name,
> +			 domain_enum domain)
>  {
>    struct using_direct *renaming;
>    int defns_mark = num_defns_collected (obstackp);
>  
> +  symbol_name_matcher_ftype *name_match
> +    = language_get_symbol_name_matcher (language_def (language_ada),
> +					lookup_name);
> +

[/me again slightly surprised]

>    for (renaming = block_using (block);
>         renaming != NULL;
>         renaming = renaming->next)
>      {
>        const char *r_name;
> -      int name_match;
>  
>        /* Avoid infinite recursions: skip this renaming if we are actually
>  	 already traversing it.
>      }      	
>  }
>  
> -/* Find symbols in DOMAIN matching NAME, in BLOCK and, if FULL_SEARCH is
> -   non-zero, enclosing scope and in global scopes, returning the number of
> -   matches.  Add these to OBSTACKP.
> +/* Find symbols in DOMAIN matching LOOKUP_NAME, in BLOCK and, if
> +   FULL_SEARCH is non-zero, enclosing scope and in global scopes,
> +   returning the number of matches.  Add these to OBSTACKP.
>  
>     When FULL_SEARCH is non-zero, any non-function/non-enumeral
>     symbol match within the nest of blocks whose innermost member is BLOCK,
> @@ -5776,17 +5780,17 @@ ada_add_all_symbols (struct obstack *obstackp,
>  
>    /* Search symbols from all global blocks.  */
>   
> -  add_nonlocal_symbols (obstackp, name, domain, 1, wild_match_p);
> +  add_nonlocal_symbols (obstackp, lookup_name, domain, 1);
>  
>    /* Now add symbols from all per-file blocks if we've gotten no hits
>       (not strictly correct, but perhaps better than an error).  */
>  
>    if (num_defns_collected (obstackp) == 0)
> -    add_nonlocal_symbols (obstackp, name, domain, 0, wild_match_p);
> +    add_nonlocal_symbols (obstackp, lookup_name, domain, 0);
>  }
>  
> -/* Find symbols in DOMAIN matching NAME, in BLOCK and, if full_search is
> -   non-zero, enclosing scope and in global scopes, returning the number of
> +/* Find symbols in DOMAIN matching LOOKUP_NAME, in BLOCK and, if full_search
                                                                    ^^^^^^^^^^^
FULL_SEARCH (as for ada_add_all_symbols)?

> +   is non-zero, enclosing scope and in global scopes, returning the number of
>     matches.
>     Sets *RESULTS to point to a vector of (SYM,BLOCK) tuples,
>     indicating the symbols found and the blocks and symbol tables (if
> @@ -13920,16 +13841,113 @@ static const struct exp_descriptor ada_exp_descriptor = {
[snip]
>  
> -static symbol_name_cmp_ftype
> -ada_get_symbol_name_cmp (const char *lookup_name)

The removal of this function is not mentioned in the ChangeLog.


Wow, that was a lot of Ada. I hope that Joel can run this through Ada's
internal tests before this goes live (or he gives his approval). While I've
tried to follow along, I am desperately unversed in Ada.


> diff --git a/gdb/completer.h b/gdb/completer.h
> index f68c6dc..1958808 100644
> --- a/gdb/completer.h
> +++ b/gdb/completer.h
> @@ -68,6 +68,62 @@ struct match_list_displayer
>     calls free on each element.  */
>  typedef std::vector<gdb::unique_xmalloc_ptr<char>> completion_list;
>  
> +/* The result of a successful completion match.  When doing symbol
> +   comparison, we use the symbol search name for the symbol name match
> +   check, but the matched name that is shown to the user may be
> +   different.  For example, Ada uses encoded names for lookup, but
> +   then wants to decode the symbol name to show to the user, and also
> +   in some cases wrap the matched name in "<sym>" (meaning we can't
> +   always use the symbol's print name.  */
                                        ^

Missing closing parenthesis.

> +private:
> +  /* The completion match result.  This can either be a pointer into
> +     M_STORAGE string, or it can be a pointer into the some other
> +     string that outlives the completion matching sequence (usually, a
> +     pointer to a symbol's name.  */
                                  ^
Another missing close parenthesis.

> +  const char *m_match;
> +
> +  /* Storage a symbol comparison routine can use for generating a
> +     match result, dynamically.  The built string is only good until
> +     the next clear() call.  I.e., good until the next symbol
> +     comparison.  */
> +  std::string m_storage;
> +};
> +
> +/* Convenience aggregate holding info returned by the symbol name
> +   matching routines (see symbol_name_matcher_ftype).  */

> diff --git a/gdb/cp-support.c b/gdb/cp-support.c
> index df9a563..95e7cb8 100644
> --- a/gdb/cp-support.c
> +++ b/gdb/cp-support.c
> @@ -1592,6 +1594,41 @@ gdb_sniff_from_mangled_name (const char *mangled, char **demangled)
>    return *demangled != NULL;
>  }
>  
[snip]
> +
> +/* Implement the "la_get_symbol_name_matcher" language_defn method for
> +   C++.  */
> +

This comment should be moved to cp-support.h.

> +symbol_name_matcher_ftype *
> +cp_get_symbol_name_matcher (const lookup_name_info &lookup_name)
> +{
> +  return cp_fq_symbol_name_matches;
> +}
> +
>  /* Don't allow just "maintenance cplus".  */
>  
>  static  void
> diff --git a/gdb/cp-support.h b/gdb/cp-support.h
> index 37b281f..3a42cd6 100644
> --- a/gdb/cp-support.h
> +++ b/gdb/cp-support.h
> @@ -107,6 +107,9 @@ extern struct symbol **make_symbol_overload_list_adl (struct type **arg_types,
>  extern struct type *cp_lookup_rtti_type (const char *name,
>  					 struct block *block);
>  
> +extern symbol_name_matcher_ftype *cp_get_symbol_name_matcher
> +  (const lookup_name_info &lookup_name);
> +

[insert comment from cp-support.c]

> diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
> index 3c547c1..28236aa 100644
> --- a/gdb/dwarf2read.c
> +++ b/gdb/dwarf2read.c
> @@ -4063,10 +4065,97 @@ dw2_map_matching_symbols (struct objfile *objfile,
[snip]
> +
> +gdb_index_symbol_name_matcher::gdb_index_symbol_name_matcher
> +  (const lookup_name_info &lookup_name)
> +    : m_lookup_name (lookup_name)

While I don't object to this formatting, IIRC we are following GCC's lead,
and their coding standard says,

  If a C++ function name is long enough to cause the first function parameter
  with its type to exceed 80 characters, it should appear on the next line
  indented four spaces.

  void
  very_long_class_name::very_long_function_name (
      very_long_type_name arg)
  {
  [https://gcc.gnu.org/codingconventions.html#Member_Form]

Have I mentioned that I think we really need to create/publish our own
coding standard (or at least a C++ cookbook)? ;-)

> +{
> +  /* Prepare the vector of comparison functions upfront, to avoid
> +     doing the same work for each symbol.  Care is taken to avoid
> +     matching with the same matcher more than once if/when multiple
> +     languages use the same matcher function.  */
> +  auto &matchers = m_symbol_name_matcher_funcs;
> +  matchers.reserve (nr_languages);
> +
> +  for (int i = 0; i < nr_languages; i++)
> +    {
> +      const language_defn *lang = language_def ((enum language) i);
> +      if (lang->la_get_symbol_name_matcher != NULL)
> +	{
> +	  symbol_name_matcher_ftype *name_matcher
> +	    = lang->la_get_symbol_name_matcher (m_lookup_name);
> +
> +	  /* Don't insert the same comparison routine more than once.
> +	     Note that we do this linear walk instead of a cheaper
> +	     sorted insert, or use a std::set or something like that,
> +	     because relative order of function addresses is not
> +	     stable.  This is not a problem in practice because the
> +	     number of supported languages is low, and the cost here
> +	     is tiny compared to the number of searches we'll do
> +	     afterwards using this object.  */
> +	  if (std::find (matchers.begin (), matchers.end (), name_matcher)
> +	      == matchers.end ())
> +	    matchers.push_back (name_matcher);
> +	}
> +    }
> +  if (std::find (matchers.begin (), matchers.end (),
> +		 default_symbol_name_matcher) == matchers.end ())
> +    matchers.push_back (default_symbol_name_matcher);

Is there a reason not to do this before looping since the loop already does
duplicate elimination? [I realize the list of languages is tiny.]

> +}
> +
> +bool
> +gdb_index_symbol_name_matcher::matches (const char *symbol_name)
> +{
> +  for (auto matches_name : m_symbol_name_matcher_funcs)
> +    if (matches_name (symbol_name, m_lookup_name, NULL))
> +      return true;
> +
> +  return false;
> +}
> +
>  static void
>  dw2_expand_symtabs_matching
>    (struct objfile *objfile,
>     gdb::function_view<expand_symtabs_file_matcher_ftype> file_matcher,
> +   const lookup_name_info &lookup_name,
>     gdb::function_view<expand_symtabs_symbol_matcher_ftype> symbol_matcher,
>     gdb::function_view<expand_symtabs_exp_notify_ftype> expansion_notify,
>     enum search_domain kind)
> diff --git a/gdb/linespec.c b/gdb/linespec.c
> index 136cb65..a241419 100644
> --- a/gdb/linespec.c
> +++ b/gdb/linespec.c
> @@ -3613,14 +3614,18 @@ lookup_prefix_sym (struct linespec_state *state, VEC (symtab_ptr) *file_symtabs,
>    struct symtab *elt;
>    decode_compound_collector collector;
>  
> +  lookup_name_info lookup_name (class_name, symbol_name_match_type::FULL);
> +
>    for (ix = 0; VEC_iterate (symtab_ptr, file_symtabs, ix, elt); ++ix)
>      {
>        if (elt == NULL)
>  	{
> -	  iterate_over_all_matching_symtabs (state, class_name, STRUCT_DOMAIN,
> -					     NULL, false, collector);
> -	  iterate_over_all_matching_symtabs (state, class_name, VAR_DOMAIN,
> -					     NULL, false, collector);
> +	  iterate_over_all_matching_symtabs (state, lookup_name,
> +					     STRUCT_DOMAIN, NULL, false,
> +					     collector);
> +	  iterate_over_all_matching_symtabs (state, lookup_name,
> +					     VAR_DOMAIN, NULL, false,
> +					     collector);
>  	}
>        else
>  	{

Side note (for myself, really): I think we might be able to  avoid doing both
STRUCT_DOMAIN and VAR_DOMAIN searches for several languages, including C++,
because of the (vile) behavior of symbol_matches_domain. It might be a
little optimization (which would need careful documenting if I should ever
manage to delete symbol_matches_domain).

> diff --git a/gdb/minsyms.c b/gdb/minsyms.c
> index c93eaa3..56fb5b4 100644
> --- a/gdb/minsyms.c
> +++ b/gdb/minsyms.c

> +/* Worker object for lookup_minimal_symbol.  Stores temporary results
> +   while walking the symbol tables.  */
> +
> +struct found_minimal_symbols
> +{
> +  /* External symbols are best.  */
> +  bound_minimal_symbol external_symbol {};
> +
> +  /* File-local symbols are next best.  */
> +  bound_minimal_symbol file_symbol {};
> +
> +  /* Symbols for shared library trampolines are next best.  */
> +  bound_minimal_symbol trampoline_symbol {};
> +
> +  /* Called when a symbol name matches.  Check if the minsym is a
> +     better type than what we had already found, and record it in one
> +     of the members fields if so.  Returns true if we already have the
> +     real symbol.  */
> +  bool maybe_collect (const char *sfile, objfile *objf,
> +		      minimal_symbol *msymbol);

I apologize, but this is a nit; don't feel obligated to change anything.
The return value to this function does not obviously map to its purpose.

Could the comment be amended/changed to explicitly mention that it means one
should stop searching for better alternatives?

> +};
> +
> +bool
> +found_minimal_symbols::maybe_collect (const char *sfile,
> +                                     struct objfile *objfile,
> +                                     minimal_symbol *msymbol)

Serious question: Do we need to put "See minsyms.h" here or are we going to
start assuming that all member definitions are documented in the
corresponding declaration? I wouldn't mind that simplification, but I thought
I'd mention it.

> diff --git a/gdb/psymtab.c b/gdb/psymtab.c
> index 4077fb3..fb0a55d 100644
> --- a/gdb/psymtab.c
> +++ b/gdb/psymtab.c
> @@ -542,6 +544,18 @@ psym_lookup_symbol (struct objfile *objfile,
>    return stab_best;
>  }
>  
> +/* Returns true if PSYM matches LOOKUP_NAME.  */
> +
> +static bool
> +psymbol_name_matches (partial_symbol *psym,
> +		      const lookup_name_info &lookup_name)
> +{
> +  const language_defn *lang = language_def (SYMBOL_LANGUAGE (psym));
> +  symbol_name_matcher_ftype *name_match
> +    = language_get_symbol_name_matcher (lang, lookup_name);
> +  return name_match (SYMBOL_SEARCH_NAME (psym), lookup_name, NULL);
> +}
> +

This is called in tight loops while searching for matching psymbols.
If there is some merit to the assumption (?!) that large amounts of psymbols
within any given objfile are the same language, this seems ripe for some
sort of trivial caching optimization. Perhaps, though, this is an
"optimization" that will have no impact on performance. [Or maybe you've
addressed this already in a future patch. Or I've missed a use case.]

>  static bool
>  recursively_search_psymtabs
> -  (struct partial_symtab *ps, struct objfile *objfile, enum search_domain kind,
> +  (struct partial_symtab *ps, struct objfile *objfile, enum search_domain domain,
> +   const lookup_name_info &lookup_name,
>     gdb::function_view<expand_symtabs_symbol_matcher_ftype> sym_matcher)

I am not bothered by renaming parameters for consistency, but it does, IMO,
deserve mention in the ChangeLog when it is not necessitated by some other
change, e.g., "name" -> "lookup_name".

>  {
>    struct partial_symbol **psym;
> @@ -1381,9 +1412,10 @@ static void
>  psym_expand_symtabs_matching
>    (struct objfile *objfile,
>     gdb::function_view<expand_symtabs_file_matcher_ftype> file_matcher,
> +   const lookup_name_info &lookup_name,
>     gdb::function_view<expand_symtabs_symbol_matcher_ftype> symbol_matcher,
>     gdb::function_view<expand_symtabs_exp_notify_ftype> expansion_notify,
> -   enum search_domain kind)
> +   enum search_domain domain)
>  {
>    struct partial_symtab *ps;
>  

Likewise

> diff --git a/gdb/symtab.c b/gdb/symtab.c
> index ba2c559..6914f69 100644
> --- a/gdb/symtab.c
> +++ b/gdb/symtab.c
> @@ -1751,6 +1766,30 @@ fixup_symbol_section (struct symbol *sym, struct objfile *objfile)
>    return sym;
>  }
>  
> +/* See symtab.h.  */
> +
> +demangle_for_lookup_info::demangle_for_lookup_info (const lookup_name_info &lookup_name,
> +						    language lang)

line length == 88

[snip]

> @@ -4755,20 +4816,35 @@ compare_symbol_name (const char *name, const char *sym_text, int sym_text_len)
>    return 1;
>  }
>  
> -/*  Test to see if the symbol specified by SYMNAME (which is already
> -   demangled for C++ symbols) matches SYM_TEXT in the first SYM_TEXT_LEN
> -   characters.  If so, add it to the current completion list.  */
> +/*  Test to see if the symbol of language SYMBOL_LANGUAGE specified by
> +    SYMNAME (which is already demangled for C++ symbols) matches
> +    SYM_TEXT in the first SYM_TEXT_LEN characters.  If so, add it to
> +    the current completion list.  */
>  
> -static void
> +void
>  completion_list_add_name (completion_tracker &tracker,

This comment should have been moved to symtab.h along with the decl.

> +			  language symbol_language,
>  			  const char *symname,
> +			  const lookup_name_info &lookup_name,
>  			  const char *sym_text, int sym_text_len,
>  			  const char *text, const char *word)
>  {
> +  completion_match_result &match_res
> +    = tracker.reset_completion_match_result ();
> +
>    /* Clip symbols that cannot match.  */
> -  if (!compare_symbol_name (symname, sym_text, sym_text_len))
> +  if (!compare_symbol_name (symname, symbol_language,
> +			    lookup_name,
> +			    sym_text, sym_text_len,
> +			    match_res))
>      return;
>  
> +  /* Refresh SYMNAME from the match string.  It's potentially
> +     different depending on language.  (E.g., on Ada, the match may be
> +     the encoded symbol name wrapped in "<>").  */
> +  symname = match_res.match.match ();
> +  gdb_assert (symname != NULL);
> +
>    /* We have a match for a completion, so add SYMNAME to the current list
>       of matches.  Note that the name is moved to freshly malloc'd space.  */
>  
> +

I think you meant to clear the previous line (which is needlessly indented),
no?

[snip]

> diff --git a/gdb/symtab.h b/gdb/symtab.h
> index da5cf25..fe7c1c4 100644
> --- a/gdb/symtab.h
> +++ b/gdb/symtab.h
> @@ -43,6 +45,249 @@ struct probe;
[snip]
> +
> +extern unsigned int search_name_hash (enum language language,
> +				      const char *search_name);
> +

symtab.c says, "See symtab.h." The above is missing that comment.

> +/* Ada-specific bits of a lookup_name_info object.  This is lazily
> +   constructed on demand.  */
> +
> +class ada_lookup_name_info final
> +{
> + public:
> +  /* Construct.  */

Just a side question... Are these types of comments particularly useful? I
realize we (used to?) have a rather strict "document every function" policy, but
(nearly) trivial ctors and (especially) dtors?

> +  explicit ada_lookup_name_info (const lookup_name_info &lookup_name);
> +

> +/* Object that aggregates all information relative to a symbol lookup
                                             ^^^^^^^^
I think you mean "relevant" here?

> +   name.  I.e., the name that is matched against the symbol's search
> +   name.  Caches per-language information so that it doesn't require
> +   recomputing it for every symbol comparison, like for example the
> +   Ada encoded name and the symbol's name hash for a given language.
> +   The object is conceptually immutable once constructed, and thus has
> +   no setters.  This is to avoid the case of a code path tweaking some
> +   property of the lookup name for some local reason, and accidentally
> +   causing other parts of symbol search that continue searching using
> +   the same lookup name be affected.  lookup_name_info objects are
> +   generally passed around as a const reference to reinforce that.
> +   (They're not passed around by value because they're not small.)  */

May I suggest rewording that one sentence:

  This is to prevent some code path from tweaking some property of the
  lookup name for some local reason and accidentally altering the results
  of any continuing search(es).

Or something like that. The fragment "... using the same lookup name be
affected" doesn't make sense (grammatically).

> +class lookup_name_info final
> +{
> + public:
> +  /* Create a new object.  */
> +  lookup_name_info (std::string name,
> +		    symbol_name_match_type match_type,
> +		    bool completion_mode = false)
> +    : m_match_type (match_type),
> +      m_completion_mode (completion_mode),
> +      m_name (std::move (name))
> +  {}
> +
> +  /* Getters.  See description of each corresponding field.  */
> +  symbol_name_match_type match_type () const { return m_match_type; }
> +  bool completion_mode () const { return m_completion_mode; }
> +  const std::string &name () const { return m_name; }

For the record, I don't have a problem with this style of commenting multiple
simplistic methods. I hope we codify this (at least) for simple
getter/setter-type methods.

[snip]

> @@ -1700,4 +1953,11 @@ void initialize_objfile_symbol (struct symbol *);
>  
>  struct template_symbol *allocate_template_symbol (struct objfile *);
>  
> +void completion_list_add_name (completion_tracker &tracker,
> +			       language symbol_language,
> +			       const char *symname,
> +			       const lookup_name_info &lookup_name,
> +			       const char *sym_text, int sym_text_len,
> +			       const char *text, const char *word);
> +

[comment from symtab.c goes here]

> diff --git a/gdb/utils.h b/gdb/utils.h
> index 48330a1..c3d707c 100644
> --- a/gdb/utils.h
> +++ b/gdb/utils.h
> @@ -31,6 +31,33 @@ extern void initialize_utils (void);
>  
>  extern int sevenbit_strings;
>  
> +/* Modes of operation for strncmp_iw_with_mode.  */
> +
> +enum class strncmp_iw_mode
> +{
> +/* Do a strcmp() type operation on STRING1 and STRING2, ignoring any
> +   differences in whitespace.  Returns 0 if they match, non-zero if they
> +   don't (slightly different than strcmp()'s range of return values).
> +
> +   As an extra hack, string1=="FOO(ARGS)" matches string2=="FOO".
> +   This "feature" is useful when searching for matching C++ function names
> +   (such as if the user types 'break FOO', where FOO is a mangled C++
> +   function).  */
> +  NORMAL,

Isn't this described "hack" the same as the hack for MATCH_PARAMS:

> +
> +  /* Like NORMAL, but also apply the strcmp_iw hack.  I.e.,
> +     string1=="FOO(PARAMS)" matches string2=="FOO".  */
> +  MATCH_PARAMS,
> +};

Maybe it's a cut-n-paste-o?

Keith