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