[PING] Re: [PATCH] C++: fix ordering of missing std #include suggestion (PR c++/81514)
Jason Merrill
jason@redhat.com
Fri Aug 18 03:21:00 GMT 2017
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
More information about the Gcc-patches
mailing list