Message ID | 1501191403-11349-1-git-send-email-dmalcolm@redhat.com |
---|---|
State | New |
Headers | show |
> I'm not sure why Solaris' decl of std::sprintf doesn't hit the > reject path above. > > I was able to reproduce the behavior seen on Solaris on my Fedora > box by using this: > > namespace std > { > extern int sprintf (char *dst, const char *format, ...); > } This is how C library symbols were intended to be declared in the C++ <cxxx> headers (plus extern "C" that GCC doesn't implement). Very few systems went to the trouble to make those changes. Solaris was one of them. Martin
Ping On Thu, 2017-07-27 at 17:36 -0400, David Malcolm wrote: > PR c++/81514 reports a problem where > g++.dg/lookup/missing-std-include-2.C > fails on Solaris, offering the suggestion: > > error: 'string' is not a member of 'std' > note: suggested alternative: 'sprintf' > > instead of the expected: > > error: 'string' is not a member of 'std' > note: 'std::string' is defined in header '<string>'; did you forget > to '#include <string>'? > > This is after a: > #include <stdio.h> > > suggest_alternative_in_explicit_scope currently works in two phases: > > (a) it attempts to look for misspellings within the explicitly-given > namespace and suggests the best it finds > > (b) failing that, it then looks for well-known "std::" > names and suggests a missing header > > This now seems the wrong way round to me; if the user has > typed "std::string", a missing #include <string> seems more helpful > as a suggestion than attempting to look for misspellings. > > This patch reverses the ordering of (a) and (b) above, so that > missing header hints for well-known std:: names are offered first, > only then falling back to misspelling hints. > > The problem doesn't show up on my x86_64-pc-linux-gnu box, as > the pertinent part of the #include <stdio.h> appears to be > equivalent to: > > extern int sprintf (char *dst, const char *format, ...); > namespace std > { > using ::sprintf; > } > > The "std::sprintf" thus examined within consider_binding_level > is the same tree node as ::sprintf, and is rejected by: > > /* Skip anticipated decls of builtin functions. */ > if (TREE_CODE (d) == FUNCTION_DECL > && DECL_BUILT_IN (d) > && DECL_ANTICIPATED (d)) > continue; > > and so the name "sprintf" is never considered as a spell-correction > for std::"string". > > Hence we're not issuing spelling corrections for aliases > within a namespace for builtins from the global namespace; > these are pre-created by cxx_builtin_function, which has: > > 4397 /* All builtins that don't begin with an '_' should > additionally > 4398 go in the 'std' namespace. */ > 4399 if (name[0] != '_') > 4400 { > 4401 tree decl2 = copy_node(decl); > 4402 push_namespace (std_identifier); > 4403 builtin_function_1 (decl2, std_node, false); > 4404 pop_namespace (); > 4405 } > > I'm not sure why Solaris' decl of std::sprintf doesn't hit the > reject path above. > > I was able to reproduce the behavior seen on Solaris on my Fedora > box by using this: > > namespace std > { > extern int sprintf (char *dst, const char *format, ...); > } > > which isn't rejected by the "Skip anticipated decls of builtin > functions" test above, and hence sprintf is erroneously offered > as a suggestion. > > The patch reworks the test case to work in the above way, > to trigger the problem on Linux, and then fixes it by > changing the order that the suggestions are tried in > name-lookup.c. It introduces an "empty.h" since the testcase > is also to verify that we suggest a good location for new #include > directives relative to pre-existing #include directives. > > Successfully bootstrapped®rtested on x86_64-pc-linux-gnu. > > OK for trunk? > > gcc/cp/ChangeLog: > PR c++/81514 > * name-lookup.c (maybe_suggest_missing_header): Convert return > type from void to bool; return true iff a suggestion was > offered. > (suggest_alternative_in_explicit_scope): Move call to > maybe_suggest_missing_header to before use of best_match, and > return true if the former offers a suggestion. > > gcc/testsuite/ChangeLog: > PR c++/81514 > * g++.dg/lookup/empty.h: New file. > * g++.dg/lookup/missing-std-include-2.C: Replace include of > stdio.h with empty.h and a declaration of a "std::sprintf" not > based > on a built-in. > --- > gcc/cp/name-lookup.c | 39 +++++++++++- > ---------- > gcc/testsuite/g++.dg/lookup/empty.h | 1 + > .../g++.dg/lookup/missing-std-include-2.C | 11 ++++-- > 3 files changed, 29 insertions(+), 22 deletions(-) > create mode 100644 gcc/testsuite/g++.dg/lookup/empty.h > > diff --git a/gcc/cp/name-lookup.c b/gcc/cp/name-lookup.c > index cd7428a..49c4dea 100644 > --- a/gcc/cp/name-lookup.c > +++ b/gcc/cp/name-lookup.c > @@ -4838,34 +4838,34 @@ get_std_name_hint (const char *name) > return NULL; > } > > -/* Subroutine of suggest_alternative_in_explicit_scope, for use when > we have no > - suggestions to offer. > - If SCOPE is the "std" namespace, then suggest pertinent header > - files for NAME. */ > +/* If SCOPE is the "std" namespace, then suggest pertinent header > + files for NAME at LOCATION. > + Return true iff a suggestion was offered. */ > > -static void > +static bool > maybe_suggest_missing_header (location_t location, tree name, tree > scope) > { > if (scope == NULL_TREE) > - return; > + return false; > if (TREE_CODE (scope) != NAMESPACE_DECL) > - return; > + return false; > /* We only offer suggestions for the "std" namespace. */ > if (scope != std_node) > - return; > + return false; > gcc_assert (TREE_CODE (name) == IDENTIFIER_NODE); > > const char *name_str = IDENTIFIER_POINTER (name); > const char *header_hint = get_std_name_hint (name_str); > - if (header_hint) > - { > - gcc_rich_location richloc (location); > - maybe_add_include_fixit (&richloc, header_hint); > - inform_at_rich_loc (&richloc, > - "%<std::%s%> is defined in header %qs;" > - " did you forget to %<#include %s%>?", > - name_str, header_hint, header_hint); > - } > + if (!header_hint) > + return false; > + > + gcc_rich_location richloc (location); > + maybe_add_include_fixit (&richloc, header_hint); > + inform_at_rich_loc (&richloc, > + "%<std::%s%> is defined in header %qs;" > + " did you forget to %<#include %s%>?", > + name_str, header_hint, header_hint); > + return true; > } > > /* Look for alternatives for NAME, an IDENTIFIER_NODE for which name > @@ -4880,6 +4880,9 @@ suggest_alternative_in_explicit_scope > (location_t location, tree name, > /* Resolve any namespace aliases. */ > scope = ORIGINAL_NAMESPACE (scope); > > + if (maybe_suggest_missing_header (location, name, scope)) > + return true; > + > cp_binding_level *level = NAMESPACE_LEVEL (scope); > > best_match <tree, const char *> bm (name); > @@ -4895,8 +4898,6 @@ suggest_alternative_in_explicit_scope > (location_t location, tree name, > fuzzy_name); > return true; > } > - else > - maybe_suggest_missing_header (location, name, scope); > > return false; > } > diff --git a/gcc/testsuite/g++.dg/lookup/empty.h > b/gcc/testsuite/g++.dg/lookup/empty.h > new file mode 100644 > index 0000000..a057418 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/lookup/empty.h > @@ -0,0 +1 @@ > +/* empty file for use by missing-std-include-2.C. */ > diff --git a/gcc/testsuite/g++.dg/lookup/missing-std-include-2.C > b/gcc/testsuite/g++.dg/lookup/missing-std-include-2.C > index ae918f8..51c604a 100644 > --- a/gcc/testsuite/g++.dg/lookup/missing-std-include-2.C > +++ b/gcc/testsuite/g++.dg/lookup/missing-std-include-2.C > @@ -6,7 +6,12 @@ > /* This is padding (to avoid the generated patch containing DejaGnu > directives). */ > > -#include <stdio.h> > +#include "empty.h" > + > +namespace std > +{ > + extern int sprintf (char *dst, const char *format, ...); > +}; > > void test (void) > { > @@ -45,11 +50,11 @@ void test_2 (void) > @@ -7,6 +7,8 @@ > directives). */ > > - #include <stdio.h> > + #include "empty.h" > +#include <string> > +#include <iostream> > > - void test (void) > + namespace std > { > { dg-end-multiline-output "" } > #endif
OK. On Thu, Aug 17, 2017 at 7:50 AM, David Malcolm <dmalcolm@redhat.com> wrote: > Ping > > On Thu, 2017-07-27 at 17:36 -0400, David Malcolm wrote: >> PR c++/81514 reports a problem where >> g++.dg/lookup/missing-std-include-2.C >> fails on Solaris, offering the suggestion: >> >> error: 'string' is not a member of 'std' >> note: suggested alternative: 'sprintf' >> >> instead of the expected: >> >> error: 'string' is not a member of 'std' >> note: 'std::string' is defined in header '<string>'; did you forget >> to '#include <string>'? >> >> This is after a: >> #include <stdio.h> >> >> suggest_alternative_in_explicit_scope currently works in two phases: >> >> (a) it attempts to look for misspellings within the explicitly-given >> namespace and suggests the best it finds >> >> (b) failing that, it then looks for well-known "std::" >> names and suggests a missing header >> >> This now seems the wrong way round to me; if the user has >> typed "std::string", a missing #include <string> seems more helpful >> as a suggestion than attempting to look for misspellings. >> >> This patch reverses the ordering of (a) and (b) above, so that >> missing header hints for well-known std:: names are offered first, >> only then falling back to misspelling hints. >> >> The problem doesn't show up on my x86_64-pc-linux-gnu box, as >> the pertinent part of the #include <stdio.h> appears to be >> equivalent to: >> >> extern int sprintf (char *dst, const char *format, ...); >> namespace std >> { >> using ::sprintf; >> } >> >> The "std::sprintf" thus examined within consider_binding_level >> is the same tree node as ::sprintf, and is rejected by: >> >> /* Skip anticipated decls of builtin functions. */ >> if (TREE_CODE (d) == FUNCTION_DECL >> && DECL_BUILT_IN (d) >> && DECL_ANTICIPATED (d)) >> continue; >> >> and so the name "sprintf" is never considered as a spell-correction >> for std::"string". >> >> Hence we're not issuing spelling corrections for aliases >> within a namespace for builtins from the global namespace; >> these are pre-created by cxx_builtin_function, which has: >> >> 4397 /* All builtins that don't begin with an '_' should >> additionally >> 4398 go in the 'std' namespace. */ >> 4399 if (name[0] != '_') >> 4400 { >> 4401 tree decl2 = copy_node(decl); >> 4402 push_namespace (std_identifier); >> 4403 builtin_function_1 (decl2, std_node, false); >> 4404 pop_namespace (); >> 4405 } >> >> I'm not sure why Solaris' decl of std::sprintf doesn't hit the >> reject path above. >> >> I was able to reproduce the behavior seen on Solaris on my Fedora >> box by using this: >> >> namespace std >> { >> extern int sprintf (char *dst, const char *format, ...); >> } >> >> which isn't rejected by the "Skip anticipated decls of builtin >> functions" test above, and hence sprintf is erroneously offered >> as a suggestion. >> >> The patch reworks the test case to work in the above way, >> to trigger the problem on Linux, and then fixes it by >> changing the order that the suggestions are tried in >> name-lookup.c. It introduces an "empty.h" since the testcase >> is also to verify that we suggest a good location for new #include >> directives relative to pre-existing #include directives. >> >> Successfully bootstrapped®rtested on x86_64-pc-linux-gnu. >> >> OK for trunk? >> >> gcc/cp/ChangeLog: >> PR c++/81514 >> * name-lookup.c (maybe_suggest_missing_header): Convert return >> type from void to bool; return true iff a suggestion was >> offered. >> (suggest_alternative_in_explicit_scope): Move call to >> maybe_suggest_missing_header to before use of best_match, and >> return true if the former offers a suggestion. >> >> gcc/testsuite/ChangeLog: >> PR c++/81514 >> * g++.dg/lookup/empty.h: New file. >> * g++.dg/lookup/missing-std-include-2.C: Replace include of >> stdio.h with empty.h and a declaration of a "std::sprintf" not >> based >> on a built-in. >> --- >> gcc/cp/name-lookup.c | 39 +++++++++++- >> ---------- >> gcc/testsuite/g++.dg/lookup/empty.h | 1 + >> .../g++.dg/lookup/missing-std-include-2.C | 11 ++++-- >> 3 files changed, 29 insertions(+), 22 deletions(-) >> create mode 100644 gcc/testsuite/g++.dg/lookup/empty.h >> >> diff --git a/gcc/cp/name-lookup.c b/gcc/cp/name-lookup.c >> index cd7428a..49c4dea 100644 >> --- a/gcc/cp/name-lookup.c >> +++ b/gcc/cp/name-lookup.c >> @@ -4838,34 +4838,34 @@ get_std_name_hint (const char *name) >> return NULL; >> } >> >> -/* Subroutine of suggest_alternative_in_explicit_scope, for use when >> we have no >> - suggestions to offer. >> - If SCOPE is the "std" namespace, then suggest pertinent header >> - files for NAME. */ >> +/* If SCOPE is the "std" namespace, then suggest pertinent header >> + files for NAME at LOCATION. >> + Return true iff a suggestion was offered. */ >> >> -static void >> +static bool >> maybe_suggest_missing_header (location_t location, tree name, tree >> scope) >> { >> if (scope == NULL_TREE) >> - return; >> + return false; >> if (TREE_CODE (scope) != NAMESPACE_DECL) >> - return; >> + return false; >> /* We only offer suggestions for the "std" namespace. */ >> if (scope != std_node) >> - return; >> + return false; >> gcc_assert (TREE_CODE (name) == IDENTIFIER_NODE); >> >> const char *name_str = IDENTIFIER_POINTER (name); >> const char *header_hint = get_std_name_hint (name_str); >> - if (header_hint) >> - { >> - gcc_rich_location richloc (location); >> - maybe_add_include_fixit (&richloc, header_hint); >> - inform_at_rich_loc (&richloc, >> - "%<std::%s%> is defined in header %qs;" >> - " did you forget to %<#include %s%>?", >> - name_str, header_hint, header_hint); >> - } >> + if (!header_hint) >> + return false; >> + >> + gcc_rich_location richloc (location); >> + maybe_add_include_fixit (&richloc, header_hint); >> + inform_at_rich_loc (&richloc, >> + "%<std::%s%> is defined in header %qs;" >> + " did you forget to %<#include %s%>?", >> + name_str, header_hint, header_hint); >> + return true; >> } >> >> /* Look for alternatives for NAME, an IDENTIFIER_NODE for which name >> @@ -4880,6 +4880,9 @@ suggest_alternative_in_explicit_scope >> (location_t location, tree name, >> /* Resolve any namespace aliases. */ >> scope = ORIGINAL_NAMESPACE (scope); >> >> + if (maybe_suggest_missing_header (location, name, scope)) >> + return true; >> + >> cp_binding_level *level = NAMESPACE_LEVEL (scope); >> >> best_match <tree, const char *> bm (name); >> @@ -4895,8 +4898,6 @@ suggest_alternative_in_explicit_scope >> (location_t location, tree name, >> fuzzy_name); >> return true; >> } >> - else >> - maybe_suggest_missing_header (location, name, scope); >> >> return false; >> } >> diff --git a/gcc/testsuite/g++.dg/lookup/empty.h >> b/gcc/testsuite/g++.dg/lookup/empty.h >> new file mode 100644 >> index 0000000..a057418 >> --- /dev/null >> +++ b/gcc/testsuite/g++.dg/lookup/empty.h >> @@ -0,0 +1 @@ >> +/* empty file for use by missing-std-include-2.C. */ >> diff --git a/gcc/testsuite/g++.dg/lookup/missing-std-include-2.C >> b/gcc/testsuite/g++.dg/lookup/missing-std-include-2.C >> index ae918f8..51c604a 100644 >> --- a/gcc/testsuite/g++.dg/lookup/missing-std-include-2.C >> +++ b/gcc/testsuite/g++.dg/lookup/missing-std-include-2.C >> @@ -6,7 +6,12 @@ >> /* This is padding (to avoid the generated patch containing DejaGnu >> directives). */ >> >> -#include <stdio.h> >> +#include "empty.h" >> + >> +namespace std >> +{ >> + extern int sprintf (char *dst, const char *format, ...); >> +}; >> >> void test (void) >> { >> @@ -45,11 +50,11 @@ void test_2 (void) >> @@ -7,6 +7,8 @@ >> directives). */ >> >> - #include <stdio.h> >> + #include "empty.h" >> +#include <string> >> +#include <iostream> >> >> - void test (void) >> + namespace std >> { >> { dg-end-multiline-output "" } >> #endif
diff --git a/gcc/cp/name-lookup.c b/gcc/cp/name-lookup.c index cd7428a..49c4dea 100644 --- a/gcc/cp/name-lookup.c +++ b/gcc/cp/name-lookup.c @@ -4838,34 +4838,34 @@ get_std_name_hint (const char *name) return NULL; } -/* Subroutine of suggest_alternative_in_explicit_scope, for use when we have no - suggestions to offer. - If SCOPE is the "std" namespace, then suggest pertinent header - files for NAME. */ +/* If SCOPE is the "std" namespace, then suggest pertinent header + files for NAME at LOCATION. + Return true iff a suggestion was offered. */ -static void +static bool maybe_suggest_missing_header (location_t location, tree name, tree scope) { if (scope == NULL_TREE) - return; + return false; if (TREE_CODE (scope) != NAMESPACE_DECL) - return; + return false; /* We only offer suggestions for the "std" namespace. */ if (scope != std_node) - return; + return false; gcc_assert (TREE_CODE (name) == IDENTIFIER_NODE); const char *name_str = IDENTIFIER_POINTER (name); const char *header_hint = get_std_name_hint (name_str); - if (header_hint) - { - gcc_rich_location richloc (location); - maybe_add_include_fixit (&richloc, header_hint); - inform_at_rich_loc (&richloc, - "%<std::%s%> is defined in header %qs;" - " did you forget to %<#include %s%>?", - name_str, header_hint, header_hint); - } + if (!header_hint) + return false; + + gcc_rich_location richloc (location); + maybe_add_include_fixit (&richloc, header_hint); + inform_at_rich_loc (&richloc, + "%<std::%s%> is defined in header %qs;" + " did you forget to %<#include %s%>?", + name_str, header_hint, header_hint); + return true; } /* Look for alternatives for NAME, an IDENTIFIER_NODE for which name @@ -4880,6 +4880,9 @@ suggest_alternative_in_explicit_scope (location_t location, tree name, /* Resolve any namespace aliases. */ scope = ORIGINAL_NAMESPACE (scope); + if (maybe_suggest_missing_header (location, name, scope)) + return true; + cp_binding_level *level = NAMESPACE_LEVEL (scope); best_match <tree, const char *> bm (name); @@ -4895,8 +4898,6 @@ suggest_alternative_in_explicit_scope (location_t location, tree name, fuzzy_name); return true; } - else - maybe_suggest_missing_header (location, name, scope); return false; } diff --git a/gcc/testsuite/g++.dg/lookup/empty.h b/gcc/testsuite/g++.dg/lookup/empty.h new file mode 100644 index 0000000..a057418 --- /dev/null +++ b/gcc/testsuite/g++.dg/lookup/empty.h @@ -0,0 +1 @@ +/* empty file for use by missing-std-include-2.C. */ diff --git a/gcc/testsuite/g++.dg/lookup/missing-std-include-2.C b/gcc/testsuite/g++.dg/lookup/missing-std-include-2.C index ae918f8..51c604a 100644 --- a/gcc/testsuite/g++.dg/lookup/missing-std-include-2.C +++ b/gcc/testsuite/g++.dg/lookup/missing-std-include-2.C @@ -6,7 +6,12 @@ /* This is padding (to avoid the generated patch containing DejaGnu directives). */ -#include <stdio.h> +#include "empty.h" + +namespace std +{ + extern int sprintf (char *dst, const char *format, ...); +}; void test (void) { @@ -45,11 +50,11 @@ void test_2 (void) @@ -7,6 +7,8 @@ directives). */ - #include <stdio.h> + #include "empty.h" +#include <string> +#include <iostream> - void test (void) + namespace std { { dg-end-multiline-output "" } #endif