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
