diff mbox series

c++: Hash table iteration for namespace-member spelling suggestions

Message ID 86c6a298-4f6b-aeb2-a9bb-dac5015d7f30@acm.org
State New
Headers show
Series c++: Hash table iteration for namespace-member spelling suggestions | expand

Commit Message

Nathan Sidwell Oct. 2, 2020, 6:23 p.m. UTC
For 'no such binding' errors, we iterate over binding levels to find a
close match.  At the namespace level we were using DECL_ANTICIPATED to
skip undeclared builtins.  But (a) there are other unnameable things
there and (b) decl-anticipated is about to go away.  This changes the
namespace scanning to iterate over the hash table, and look at
non-hidden bindings.  This does mean we look at fewer strings
(hurrarh), but the order we meet them is somewhat 'random'.  Our
distance measure is not very fine grained, and a couple of testcases
change their suggestion.  I notice for the c/c++ common one, we now
match the output of the C compiler.  For the other one we think 'int'
and 'int64_t' have the same distance from 'int64', and now meet the
former first.  That's a little unfortunate.  If it's too problematic I
suppose we could sort the strings via an intermediate array before
measuring distance.

         gcc/cp/
         * name-lookup.c (consider_decl): New, broken out of ...
         (consider_binding_level): ... here.  Iterate the hash table for
         namespace bindings.
         gcc/testsuite/
         * c-c++-common/spellcheck-reserved.c: Adjust diagnostic.
         * g++.dg/spellcheck-typenames.C: Adjust diagnostic.

pushing to trunk

nathan

Comments

Christophe Lyon Oct. 5, 2020, 9:48 a.m. UTC | #1
On Fri, 2 Oct 2020 at 20:23, Nathan Sidwell <nathan@acm.org> wrote:
>
>
> For 'no such binding' errors, we iterate over binding levels to find a
> close match.  At the namespace level we were using DECL_ANTICIPATED to
> skip undeclared builtins.  But (a) there are other unnameable things
> there and (b) decl-anticipated is about to go away.  This changes the
> namespace scanning to iterate over the hash table, and look at
> non-hidden bindings.  This does mean we look at fewer strings
> (hurrarh), but the order we meet them is somewhat 'random'.  Our
> distance measure is not very fine grained, and a couple of testcases
> change their suggestion.  I notice for the c/c++ common one, we now
> match the output of the C compiler.  For the other one we think 'int'
> and 'int64_t' have the same distance from 'int64', and now meet the
> former first.  That's a little unfortunate.  If it's too problematic I
> suppose we could sort the strings via an intermediate array before
> measuring distance.
>
>          gcc/cp/
>          * name-lookup.c (consider_decl): New, broken out of ...
>          (consider_binding_level): ... here.  Iterate the hash table for
>          namespace bindings.
>          gcc/testsuite/
>          * c-c++-common/spellcheck-reserved.c: Adjust diagnostic.
>          * g++.dg/spellcheck-typenames.C: Adjust diagnostic.
>
> pushing to trunk
>

Hi Nathan,

This is causing regressions on aarch64 and arm when using
-mfloat-abi=hard (or configuring for arm-linux-gnueabihf).
The logs says:
FAIL: c-c++-common/spellcheck-reserved.c  -std=gnu++98 (test for excess errors)
Excess errors:
/gcc/testsuite/c-c++-common/spellcheck-reserved.c:31:3: error:
'__builtin_strtchr' was not declared in this scope; did you mean
'__builtin_strrchr'?

The test still passes on arm with -mfloat=abi=soft

Christophe

> nathan
>
> --
> Nathan Sidwell
Jakub Jelinek Oct. 5, 2020, 9:57 a.m. UTC | #2
On Mon, Oct 05, 2020 at 11:48:32AM +0200, Christophe Lyon via Gcc-patches wrote:
> > For 'no such binding' errors, we iterate over binding levels to find a
> > close match.  At the namespace level we were using DECL_ANTICIPATED to
> > skip undeclared builtins.  But (a) there are other unnameable things
> > there and (b) decl-anticipated is about to go away.  This changes the
> > namespace scanning to iterate over the hash table, and look at
> > non-hidden bindings.  This does mean we look at fewer strings
> > (hurrarh), but the order we meet them is somewhat 'random'.  Our
> > distance measure is not very fine grained, and a couple of testcases
> > change their suggestion.  I notice for the c/c++ common one, we now
> > match the output of the C compiler.  For the other one we think 'int'
> > and 'int64_t' have the same distance from 'int64', and now meet the
> > former first.  That's a little unfortunate.  If it's too problematic I
> > suppose we could sort the strings via an intermediate array before
> > measuring distance.
> >
> >          gcc/cp/
> >          * name-lookup.c (consider_decl): New, broken out of ...
> >          (consider_binding_level): ... here.  Iterate the hash table for
> >          namespace bindings.
> >          gcc/testsuite/
> >          * c-c++-common/spellcheck-reserved.c: Adjust diagnostic.
> >          * g++.dg/spellcheck-typenames.C: Adjust diagnostic.
> >
> > pushing to trunk
> >
> 
> Hi Nathan,
> 
> This is causing regressions on aarch64 and arm when using
> -mfloat-abi=hard (or configuring for arm-linux-gnueabihf).
> The logs says:
> FAIL: c-c++-common/spellcheck-reserved.c  -std=gnu++98 (test for excess errors)
> Excess errors:
> /gcc/testsuite/c-c++-common/spellcheck-reserved.c:31:3: error:
> '__builtin_strtchr' was not declared in this scope; did you mean
> '__builtin_strrchr'?

Yeah, ditto on i686-linux (but passes on x86_64-linux).
Iterating over a hash table is fine, but we should not generate random
results.  So, the function that decides what is better and worse needs to
have some additional rules, so that no two different decl names are considered
equal.  Whether it is DECL_UID, or string length of the name, or some hash
of the string characters doesn't matter that much.

	Jakub
Nathan Sidwell Oct. 5, 2020, 11:52 a.m. UTC | #3
On 10/5/20 5:48 AM, Christophe Lyon wrote:
> On Fri, 2 Oct 2020 at 20:23, Nathan Sidwell <nathan@acm.org> wrote:
>>

> Hi Nathan,
> 
> This is causing regressions on aarch64 and arm when using
> -mfloat-abi=hard (or configuring for arm-linux-gnueabihf).
> The logs says:
> FAIL: c-c++-common/spellcheck-reserved.c  -std=gnu++98 (test for excess errors)
> Excess errors:
> /gcc/testsuite/c-c++-common/spellcheck-reserved.c:31:3: error:
> '__builtin_strtchr' was not declared in this scope; did you mean
> '__builtin_strrchr'?
> 
> The test still passes on arm with -mfloat=abi=soft

thanks.  What I'd ignored is that other targets (or abi-variants) might 
have different builtins.  Those would perturb the hash table order. 
I'll have to do it right ...

nathan
diff mbox series

Patch

diff --git i/gcc/cp/name-lookup.c w/gcc/cp/name-lookup.c
index 6204444f3a6..4024ceaa74b 100644
--- i/gcc/cp/name-lookup.c
+++ w/gcc/cp/name-lookup.c
@@ -6106,6 +6106,39 @@  qualified_namespace_lookup (tree scope, name_lookup *lookup)
   return found;
 }
 
+static void
+consider_decl (tree decl,  best_match <tree, const char *> &bm,
+	       bool consider_impl_names)
+{
+  /* Skip compiler-generated variables (e.g. __for_begin/__for_end
+     within range for).  */
+  if (TREE_CODE (decl) == VAR_DECL && DECL_ARTIFICIAL (decl))
+    return;
+
+  tree suggestion = DECL_NAME (decl);
+  if (!suggestion)
+    return;
+
+  /* Don't suggest names that are for anonymous aggregate types, as
+     they are an implementation detail generated by the compiler.  */
+  if (IDENTIFIER_ANON_P (suggestion))
+    return;
+
+  const char *suggestion_str = IDENTIFIER_POINTER (suggestion);
+
+  /* Ignore internal names with spaces in them.  */
+  if (strchr (suggestion_str, ' '))
+    return;
+
+  /* Don't suggest names that are reserved for use by the
+     implementation, unless NAME began with an underscore.  */
+  if (!consider_impl_names
+      && name_reserved_for_implementation_p (suggestion_str))
+    return;
+
+  bm.consider (suggestion_str);
+}
+
 /* Helper function for lookup_name_fuzzy.
    Traverse binding level LVL, looking for good name matches for NAME
    (and BM).  */
@@ -6129,54 +6162,63 @@  consider_binding_level (tree name, best_match <tree, const char *> &bm,
      with an underscore.  */
   bool consider_implementation_names = (IDENTIFIER_POINTER (name)[0] == '_');
 
-  for (tree t = lvl->names; t; t = TREE_CHAIN (t))
-    {
-      tree d = t;
-
-      /* OVERLOADs or decls from using declaration are wrapped into
-	 TREE_LIST.  */
-      if (TREE_CODE (d) == TREE_LIST)
-	d = OVL_FIRST (TREE_VALUE (d));
-
-      /* Don't use bindings from implicitly declared functions,
-	 as they were likely misspellings themselves.  */
-      if (TREE_TYPE (d) == error_mark_node)
-	continue;
-
-      /* Skip anticipated decls of builtin functions.  */
-      if (TREE_CODE (d) == FUNCTION_DECL
-	  && fndecl_built_in_p (d)
-	  && DECL_ANTICIPATED (d))
-	continue;
+  if (lvl->kind != sk_namespace)
+    for (tree t = lvl->names; t; t = TREE_CHAIN (t))
+      {
+	tree d = t;
 
-      /* Skip compiler-generated variables (e.g. __for_begin/__for_end
-	 within range for).  */
-      if (TREE_CODE (d) == VAR_DECL
-	  && DECL_ARTIFICIAL (d))
-	continue;
+	/* OVERLOADs or decls from using declaration are wrapped into
+	   TREE_LIST.  */
+	if (TREE_CODE (d) == TREE_LIST)
+	  d = OVL_FIRST (TREE_VALUE (d));
 
-      tree suggestion = DECL_NAME (d);
-      if (!suggestion)
-	continue;
-
-      /* Don't suggest names that are for anonymous aggregate types, as
-	 they are an implementation detail generated by the compiler.  */
-      if (IDENTIFIER_ANON_P (suggestion))
-	continue;
+	/* Don't use bindings from implicitly declared functions,
+	   as they were likely misspellings themselves.  */
+	if (TREE_TYPE (d) == error_mark_node)
+	  continue;
 
-      const char *suggestion_str = IDENTIFIER_POINTER (suggestion);
+	/* If we want a typename, ignore non-types.  */
+	if (kind == FUZZY_LOOKUP_TYPENAME
+	    && TREE_CODE (STRIP_TEMPLATE (d)) != TYPE_DECL)
+	  continue;
 
-      /* Ignore internal names with spaces in them.  */
-      if (strchr (suggestion_str, ' '))
-	continue;
+	consider_decl (d, bm, consider_implementation_names);
+      }
+  else
+    {
+      /* Iterate over the namespace hash table, that'll have fewer
+	 entries than the decl list.  */
+      tree ns = lvl->this_entity;
 
-      /* Don't suggest names that are reserved for use by the
-	 implementation, unless NAME began with an underscore.  */
-      if (name_reserved_for_implementation_p (suggestion_str)
-	  && !consider_implementation_names)
-	continue;
+      hash_table<named_decl_hash>::iterator end
+	(DECL_NAMESPACE_BINDINGS (ns)->end ());
+      for (hash_table<named_decl_hash>::iterator iter
+	     (DECL_NAMESPACE_BINDINGS (ns)->begin ()); iter != end; ++iter)
+	{
+	  tree binding = *iter;
+	  tree value = NULL_TREE;
 
-      bm.consider (suggestion_str);
+	  if (STAT_HACK_P (binding))
+	    {
+	      if (!STAT_TYPE_HIDDEN_P (binding)
+		  && STAT_TYPE (binding))
+		consider_decl (STAT_TYPE (binding), bm,
+			       consider_implementation_names);
+	      else if (!STAT_DECL_HIDDEN_P (binding))
+		value = STAT_DECL (binding);
+	    }
+	  else
+	    value = binding;
+	  
+	  value = ovl_skip_hidden (value);
+	  if (value)
+	    {
+	      value = OVL_FIRST (value);
+	      if (!(kind == FUZZY_LOOKUP_TYPENAME
+		    && TREE_CODE (STRIP_TEMPLATE (value)) != TYPE_DECL))
+		consider_decl (value, bm, consider_implementation_names);
+	    }
+	}
     }
 }
 
diff --git i/gcc/testsuite/c-c++-common/spellcheck-reserved.c w/gcc/testsuite/c-c++-common/spellcheck-reserved.c
index ed292f2bae0..175ba4a2e0f 100644
--- i/gcc/testsuite/c-c++-common/spellcheck-reserved.c
+++ w/gcc/testsuite/c-c++-common/spellcheck-reserved.c
@@ -30,7 +30,7 @@  void test (const char *buf, char ch)
 {
   __builtin_strtchr (buf, ch); /* { dg-line misspelled_reserved } */
   /* { dg-warning "did you mean '__builtin_strchr'" "" { target c } misspelled_reserved } */
-  /* { dg-error "'__builtin_strtchr' was not declared in this scope; did you mean '__builtin_strrchr'\\?" "" { target c++ } misspelled_reserved } */
+  /* { dg-error "'__builtin_strtchr' was not declared in this scope; did you mean '__builtin_strchr'\\?" "" { target c++ } misspelled_reserved } */
 }
 
 /* Similarly for a name that begins with a single underscore.  */
diff --git i/gcc/testsuite/g++.dg/spellcheck-typenames.C w/gcc/testsuite/g++.dg/spellcheck-typenames.C
index 6adf724a3b7..ff53ecc6303 100644
--- i/gcc/testsuite/g++.dg/spellcheck-typenames.C
+++ w/gcc/testsuite/g++.dg/spellcheck-typenames.C
@@ -54,11 +54,11 @@  struct some_thing test_6; // { dg-error "aggregate 'some_thing test_6' has incom
    { dg-end-multiline-output "" } */
 
 typedef long int64_t;
-int64 i; // { dg-error "1: 'int64' does not name a type; did you mean 'int64_t'?" }
+int64 i; // { dg-error "1: 'int64' does not name a type; did you mean 'int'?" }
 /* { dg-begin-multiline-output "" }
  int64 i;
  ^~~~~
- int64_t
+ int
    { dg-end-multiline-output "" } */
 
 /* Verify that gcc doesn't offer nonsensical suggestions.  */