| Message ID | 20101202191043.GC18963@nightcrawler |
|---|---|
| State | New |
| Headers | show |
Hello Nathan, Nathan Froyd <froydnj@codesourcery.com> writes: [...] > @@ -449,7 +449,10 @@ unqualified_name_lookup_error (tree name) > else > { > if (!objc_diagnose_private_ivar (name)) > - error ("%qD was not declared in this scope", name); > + { > + error ("%qD was not declared in this scope", name); > + suggest_alternatives_for (name); > + } As we are gradually moving away from using the global input_location variable for better diagnostics maybe suggest_alternative could take a location parameter that ... [...] > +void > +suggest_alternatives_for (tree name) > +{ [...] > + if (n_searched >= max_to_search) > + inform (input_location, > + "maximum limit of %d namespaces searched for %qE", > + max_to_search, name); ... would be used here instead of using the global input_location. OK, this really is a small nit. Sorry for the noise if it's irrelevant. In any case, thank you for working on this.
On Fri, Dec 03, 2010 at 11:22:45AM +0100, Dodji Seketeli wrote: > Nathan Froyd <froydnj@codesourcery.com> writes: > > @@ -449,7 +449,10 @@ unqualified_name_lookup_error (tree name) > > else > > { > > if (!objc_diagnose_private_ivar (name)) > > - error ("%qD was not declared in this scope", name); > > + { > > + error ("%qD was not declared in this scope", name); > > + suggest_alternatives_for (name); > > + } > > As we are gradually moving away from using the global input_location > variable for better diagnostics maybe suggest_alternative could take a > location parameter that ... > > > +void > > +suggest_alternatives_for (tree name) > > +{ > > [...] > > > + if (n_searched >= max_to_search) > > + inform (input_location, > > + "maximum limit of %d namespaces searched for %qE", > > + max_to_search, name); > > ... would be used here instead of using the global input_location. > > OK, this really is a small nit. Sorry for the noise if it's > irrelevant. I think that's a good suggestion; I wasn't aware that we were trying to move away from input_location. One thing that really threw me from the diagnostics is that the C++ language-specifier formatter can set the location of the message (error.c:cp_printer). That seems a little weird--it would be better if the location were explicit at the point of the diagnostic, rather than arising from the things being printed. I can do this pretty easily, but I think it will require unstatic'ing location_of from error.c as my patch for PR 45329 does. It doesn't matter to me which goes in first; I suppose I can add the location_of bits to whichever one does. -Nathan
On Fri, Dec 3, 2010 at 10:41 AM, Nathan Froyd <froydnj@codesourcery.com> wrote: > On Fri, Dec 03, 2010 at 11:22:45AM +0100, Dodji Seketeli wrote: >> Nathan Froyd <froydnj@codesourcery.com> writes: >> > @@ -449,7 +449,10 @@ unqualified_name_lookup_error (tree name) >> > else >> > { >> > if (!objc_diagnose_private_ivar (name)) >> > - error ("%qD was not declared in this scope", name); >> > + { >> > + error ("%qD was not declared in this scope", name); >> > + suggest_alternatives_for (name); >> > + } >> >> As we are gradually moving away from using the global input_location >> variable for better diagnostics maybe suggest_alternative could take a >> location parameter that ... >> >> > +void >> > +suggest_alternatives_for (tree name) >> > +{ >> >> [...] >> >> > + if (n_searched >= max_to_search) >> > + inform (input_location, >> > + "maximum limit of %d namespaces searched for %qE", >> > + max_to_search, name); >> >> ... would be used here instead of using the global input_location. >> >> OK, this really is a small nit. Sorry for the noise if it's >> irrelevant. > > I think that's a good suggestion; I wasn't aware that we were trying to > move away from input_location. > > One thing that really threw me from the diagnostics is that the C++ > language-specifier formatter can set the location of the message > (error.c:cp_printer). That seems a little weird--it would be better if > the location were explicit at the point of the diagnostic, rather than > arising from the things being printed. It is far more involved than that. Locations are also retrieved from expressions. What you are seeing is remains of history. > > I can do this pretty easily, but I think it will require unstatic'ing > location_of from error.c as my patch for PR 45329 does. It doesn't > matter to me which goes in first; I suppose I can add the location_of > bits to whichever one does. > > -Nathan >
On Fri, Dec 03, 2010 at 10:48:17AM -0600, Gabriel Dos Reis wrote: > On Fri, Dec 3, 2010 at 10:41 AM, Nathan Froyd <froydnj@codesourcery.com> wrote: > > One thing that really threw me from the diagnostics is that the C++ > > language-specifier formatter can set the location of the message > > (error.c:cp_printer). That seems a little weird--it would be better if > > the location were explicit at the point of the diagnostic, rather than > > arising from the things being printed. > > It is far more involved than that. Locations are also retrieved from > expressions. What you are seeing is remains of history. Are you saying that set_locus should always be false (and therefore removed)? Because that doesn't seem to square with "Locations are also retrieved from expressions", implying that the diagnostic machinery should be getting location information from the things that it's printing. -Nathan
Nathan Froyd <froydnj@codesourcery.com> writes: [...] > > One thing that really threw me from the diagnostics is that the C++ > language-specifier formatter can set the location of the message > (error.c:cp_printer). That seems a little weird--it would be better if > the location were explicit at the point of the diagnostic, rather than > arising from the things being printed. From what I understand, it's more that cp_printer chooses a location among several locations that it gets implicitely. "Implicitely" because those locations are carried by the trees passed to cp_printer. It could be desirable to let cp_printer keep dealing with the details of choosing locations, if [for instance] we were to move to range based diagnostics. By range based diagnostics I mean being able to tell the user that erroneous construct spans from location L0 to location L1. In any case, this is maybe out of the scope of your current work, so I might as well stop distracting you :-)
On 12/03/2010 11:41 AM, Nathan Froyd wrote: > One thing that really threw me from the diagnostics is that the C++ > language-specifier formatter can set the location of the message > (error.c:cp_printer). That seems a little weird--it would be better if > the location were explicit at the point of the diagnostic, rather than > arising from the things being printed. If you say %q+D in your error message, that means "use the locus of this decl as well as printing it." That seems a lot more concise than having to write error_at (DECL_SOURCE_LOCATION (decl), "...%qD...", decl); Jason
On 12/4/2010 5:16 PM, Jason Merrill wrote: > If you say %q+D in your error message, that means "use the locus of this > decl as well as printing it." That seems a lot more concise than having > to write error_at (DECL_SOURCE_LOCATION (decl), "...%qD...", decl); 100% agreed. "%q+D" is a good thing. On the other hand, I do also agree that using input_location is not a good thing; the locations for error messages should be driven by something explicit (a declaration, or something else).
On 12/3/2010 10:41 AM, Nathan Froyd wrote: >> As we are gradually moving away from using the global input_location >> variable for better diagnostics maybe suggest_alternative could take a >> location parameter that ... >> ... would be used here instead of using the global input_location. >> >> OK, this really is a small nit. Sorry for the noise if it's >> irrelevant. > > I think that's a good suggestion; I wasn't aware that we were trying to > move away from input_location. > I can do this pretty easily, but I think it will require unstatic'ing > location_of from error.c as my patch for PR 45329 does. Please make these changes and commit; the patch is OK with those changes. Thank you,
On Sun, Dec 05, 2010 at 10:07:56AM -0600, Mark Mitchell wrote: > On 12/4/2010 5:16 PM, Jason Merrill wrote: > > If you say %q+D in your error message, that means "use the locus of this > > decl as well as printing it." That seems a lot more concise than having > > to write error_at (DECL_SOURCE_LOCATION (decl), "...%qD...", decl); > > 100% agreed. "%q+D" is a good thing. > > On the other hand, I do also agree that using input_location is not a > good thing; the locations for error messages should be driven by > something explicit (a declaration, or something else). Eh, so what's the desired way forward here? AFAICS, the `+' modifier is used only with `error' or `warning'. But both of those functions use input_location; if we want to get rid of input_location, it seems like the easiest thing (along the way; surely this is not the only thing that would be required) to do would be to excise `error' and `warning', use their _at variants, remove the `+' modifiers from the diagnostic strings, and use DECL_SOURCE_LOCATION or location_at or what have you at the point of the call. (You could do something like: perl -pi -e 's/(error|warning) \(/\1_at (UNKNOWN_LOCATION/g' **/*.c or similar, but I hope you agree that would be really ugly.) I agree that %q+D is concise, but the magicness of it is a little weird. -Nathan
On 12/06/2010 02:00 PM, Nathan Froyd wrote: > Eh, so what's the desired way forward here? AFAICS, the `+' modifier is > used only with `error' or `warning'. But both of those functions use > input_location; if we want to get rid of input_location, it seems like > the easiest thing (along the way; surely this is not the only thing that > would be required) to do would be to excise `error' and `warning', use > their _at variants, remove the `+' modifiers from the diagnostic > strings, and use DECL_SOURCE_LOCATION or location_at or what have you at > the point of the call. Or you could just require that error/warning include a '+' modfier in the format string. In any case, this sounds rather like the last change in the process of moving away from input_location, rather than the first. :) Jason
On Mon, Dec 6, 2010 at 1:05 PM, Jason Merrill <jason@redhat.com> wrote: > On 12/06/2010 02:00 PM, Nathan Froyd wrote: >> >> Eh, so what's the desired way forward here? AFAICS, the `+' modifier is >> used only with `error' or `warning'. But both of those functions use >> input_location; if we want to get rid of input_location, it seems like >> the easiest thing (along the way; surely this is not the only thing that >> would be required) to do would be to excise `error' and `warning', use >> their _at variants, remove the `+' modifiers from the diagnostic >> strings, and use DECL_SOURCE_LOCATION or location_at or what have you at >> the point of the call. > > Or you could just require that error/warning include a '+' modfier in the > format string. In any case, this sounds rather like the last change in the > process of moving away from input_location, rather than the first. :) agreed. -- Gaby
diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h index 23f594c..8bee63d 100644 --- a/gcc/cp/cp-tree.h +++ b/gcc/cp/cp-tree.h @@ -5639,6 +5639,9 @@ extern tree cxx_omp_clause_dtor (tree, tree); extern void cxx_omp_finish_clause (tree); extern bool cxx_omp_privatize_by_reference (const_tree); +/* in name-lookup.c */ +extern void suggest_alternatives_for (tree); + /* -- end of C++ */ #endif /* ! GCC_CP_TREE_H */ diff --git a/gcc/cp/error.c b/gcc/cp/error.c index 2676966..db01ba4 100644 --- a/gcc/cp/error.c +++ b/gcc/cp/error.c @@ -1699,6 +1699,7 @@ dump_expr (tree t, int flags) case NAMESPACE_DECL: case LABEL_DECL: case OVERLOAD: + case TYPE_DECL: case IDENTIFIER_NODE: dump_decl (t, (flags & ~TFF_DECL_SPECIFIERS) | TFF_NO_FUNCTION_ARGUMENTS); break; diff --git a/gcc/cp/lex.c b/gcc/cp/lex.c index 5484317..084a04b 100644 --- a/gcc/cp/lex.c +++ b/gcc/cp/lex.c @@ -449,7 +449,10 @@ unqualified_name_lookup_error (tree name) else { if (!objc_diagnose_private_ivar (name)) - error ("%qD was not declared in this scope", name); + { + error ("%qD was not declared in this scope", name); + suggest_alternatives_for (name); + } /* Prevent repeated error messages by creating a VAR_DECL with this NAME in the innermost block scope. */ if (current_function_decl) diff --git a/gcc/cp/name-lookup.c b/gcc/cp/name-lookup.c index 3d19c08..9db6c13 100644 --- a/gcc/cp/name-lookup.c +++ b/gcc/cp/name-lookup.c @@ -29,8 +29,10 @@ along with GCC; see the file COPYING3. If not see #include "name-lookup.h" #include "timevar.h" #include "diagnostic-core.h" +#include "intl.h" #include "debug.h" #include "c-family/c-pragma.h" +#include "params.h" /* The bindings for a particular name in a particular scope. */ @@ -3916,6 +3918,74 @@ remove_hidden_names (tree fns) return fns; } +/* Suggest alternatives for NAME, an IDENTIFIER_NODE for which name + lookup failed. Search through all available namespaces and print out + possible candidates. */ + +void +suggest_alternatives_for (tree name) +{ + VEC(tree,heap) *candidates = NULL; + VEC(tree,heap) *namespaces_to_search = NULL; + int max_to_search = PARAM_VALUE (CXX_MAX_NAMESPACES_FOR_DIAGNOSTIC_HELP); + int n_searched = 0; + char *spaces; + const char *str; + tree t; + unsigned ix; + + VEC_safe_push (tree, heap, namespaces_to_search, global_namespace); + + while (!VEC_empty (tree, namespaces_to_search) + && n_searched < max_to_search) + { + tree scope = VEC_pop (tree, namespaces_to_search); + struct scope_binding binding = EMPTY_SCOPE_BINDING; + struct cp_binding_level *level = NAMESPACE_LEVEL (scope); + + /* Look in this namespace. */ + qualified_lookup_using_namespace (name, scope, &binding, 0); + + n_searched++; + + if (binding.value) + VEC_safe_push (tree, heap, candidates, binding.value); + + /* Add child namespaces. */ + for (t = level->namespaces; t; t = DECL_CHAIN (t)) + VEC_safe_push (tree, heap, namespaces_to_search, t); + } + + /* If we stopped before we could examine all namespaces, inform the + user. Do this even if we don't have any candidates, since there + might be more candidates further down that we weren't able to + find. */ + if (n_searched >= max_to_search) + inform (input_location, + "maximum limit of %d namespaces searched for %qE", + max_to_search, name); + + VEC_free (tree, heap, namespaces_to_search); + + /* Nothing useful to report. */ + if (VEC_empty (tree, candidates)) + return; + + str = (VEC_length(tree, candidates) > 1 + ? _("suggested alternatives:") + : _("suggested alternative:")); + spaces = NULL; + + FOR_EACH_VEC_ELT (tree, candidates, ix, t) + { + inform (input_location, "%s %qE", (spaces ? spaces : str), t); + spaces = spaces ? spaces : get_spaces (str); + } + + VEC_free (tree, heap, candidates); + free (spaces); +} + /* Unscoped lookup of a global: iterate over current namespaces, considering using-directives. */ diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi index 94e8160..b03629c 100644 --- a/gcc/doc/invoke.texi +++ b/gcc/doc/invoke.texi @@ -8817,6 +8817,10 @@ Size of minimal paritition for WHOPR (in estimated instructions). This prevents expenses of splitting very small programs into too many partitions. +@item cxx-max-namespaces-for-diagnostic-help +The maximum number of namespaces to consult for suggestions when C++ +name lookup fails for an identifier. The default is 1000. + @end table @end table diff --git a/gcc/params.def b/gcc/params.def index 6b6e055..2ea0013 100644 --- a/gcc/params.def +++ b/gcc/params.def @@ -855,6 +855,15 @@ DEFPARAM (MIN_PARTITION_SIZE, "lto-min-partition", "Size of minimal paritition for WHOPR (in estimated instructions)", 1000, 0, 0) + +/* Diagnostic parameters. */ + +DEFPARAM (CXX_MAX_NAMESPACES_FOR_DIAGNOSTIC_HELP, + "cxx-max-namespaces-for-diagnostic-help", + "Maximum number of namespaces to search for alternatives when " + "name lookup fails", + 1000, 0, 0) + /* Local variables: mode:c diff --git a/gcc/testsuite/g++.dg/ext/builtin3.C b/gcc/testsuite/g++.dg/ext/builtin3.C index 3d06dd7..b2ec9be 100644 --- a/gcc/testsuite/g++.dg/ext/builtin3.C +++ b/gcc/testsuite/g++.dg/ext/builtin3.C @@ -10,4 +10,5 @@ extern "C" int printf(char*, ...); void foo() { printf("abc"); // { dg-error "not declared" } + // { dg-message "std::printf" "suggested alternative" { target *-*-* } 12 } } diff --git a/gcc/testsuite/g++.dg/lookup/error1.C b/gcc/testsuite/g++.dg/lookup/error1.C index 2264b23..1778dbb 100644 --- a/gcc/testsuite/g++.dg/lookup/error1.C +++ b/gcc/testsuite/g++.dg/lookup/error1.C @@ -4,6 +4,7 @@ namespace N { int i; } void foo() { i; } // { dg-error "not declared" } + // { dg-message "N::i" "suggested alternative" { target *-*-* } 6 } using namespace N; void bar() { i; } diff --git a/gcc/testsuite/g++.dg/lookup/koenig5.C b/gcc/testsuite/g++.dg/lookup/koenig5.C index 6ecc25d..1202cd7 100644 --- a/gcc/testsuite/g++.dg/lookup/koenig5.C +++ b/gcc/testsuite/g++.dg/lookup/koenig5.C @@ -32,10 +32,12 @@ void g (N::A *a, M::B *b, O::C *c) One (a); // ok One (a, b); // ok One (b); // { dg-error "not declared" } + // { dg-message "\[NM\]::One" "suggested alternative" { target *-*-* } 34 } Two (c); // ok Two (a, c); // ok Two (a); // { dg-error "not declared" } + // { dg-message "\[NMO\]::Two" "suggested alternative" { target *-*-* } 39 } Two (a, a); // error masked by earlier error Two (b); // error masked by earlier error Two (a, b); // error masked by earlier error @@ -43,4 +45,5 @@ void g (N::A *a, M::B *b, O::C *c) Three (b); // ok Three (a, b); // ok Three (a); // { dg-error "not declared" } + // { dg-message "\[NM\]::Three" "suggested alternative" { target *-*-* } 47 } } diff --git a/gcc/testsuite/g++.dg/overload/koenig1.C b/gcc/testsuite/g++.dg/overload/koenig1.C index 1ed7bce..0a64674 100644 --- a/gcc/testsuite/g++.dg/overload/koenig1.C +++ b/gcc/testsuite/g++.dg/overload/koenig1.C @@ -14,5 +14,6 @@ void g () B *bp; N::A *ap; f (bp); // { dg-error "not declared" } + // { dg-message "N::f" "suggested alternative" { target *-*-* } 16 } f (ap); } diff --git a/gcc/testsuite/g++.dg/parse/decl-specifier-1.C b/gcc/testsuite/g++.dg/parse/decl-specifier-1.C index e81fbab..092af1e 100644 --- a/gcc/testsuite/g++.dg/parse/decl-specifier-1.C +++ b/gcc/testsuite/g++.dg/parse/decl-specifier-1.C @@ -13,4 +13,5 @@ N::X X; // { dg-error "" "" } int main() { return sizeof(X); // { dg-error "" "" } + // { dg-message "N::X" "suggested alternative" { target *-*-* } 15 } } diff --git a/gcc/testsuite/g++.dg/pr45330.C b/gcc/testsuite/g++.dg/pr45330.C new file mode 100644 index 0000000..a4b22f9 --- /dev/null +++ b/gcc/testsuite/g++.dg/pr45330.C @@ -0,0 +1,19 @@ +// { dg-do compile } +// Search std, __cxxabiv1, and global namespaces, plus one more. +// { dg-options "--param cxx-max-namespaces-for-diagnostic-help=4" } + +#define NSPACE(NAME) namespace NAME { int foo; } + +NSPACE(A) +NSPACE(B) +NSPACE(C) +NSPACE(D) +NSPACE(E) + +int bar() +{ + return foo; // { dg-error "was not declared" } + // { dg-message "maximum limit of 4 namespaces searched" "" { target *-*-* } 15 } + // We don't know which namespace will come up first, so accept any. + // { dg-message "\[ABCDE\]::foo" "" { target *-*-* } 15 } +} diff --git a/gcc/testsuite/g++.dg/template/static10.C b/gcc/testsuite/g++.dg/template/static10.C index ab857bd..ce8ed9e 100644 --- a/gcc/testsuite/g++.dg/template/static10.C +++ b/gcc/testsuite/g++.dg/template/static10.C @@ -20,4 +20,5 @@ namespace std { template<> void vector<int, allocator<int> >::swap(vector<int, allocator<int> >&) { } // { dg-error "" } + // { dg-message "std::allocator" "suggested alternative" { target *-*-* } 22 } } diff --git a/gcc/testsuite/g++.old-deja/g++.mike/ns5.C b/gcc/testsuite/g++.old-deja/g++.mike/ns5.C index 9d806ca..f95821d 100644 --- a/gcc/testsuite/g++.old-deja/g++.mike/ns5.C +++ b/gcc/testsuite/g++.old-deja/g++.mike/ns5.C @@ -4,3 +4,4 @@ namespace A { } int j = i; // { dg-error "" } + // { dg-message "A::i" "suggested alternative" { target *-*-* } 6 } diff --git a/gcc/testsuite/g++.old-deja/g++.mike/ns7.C b/gcc/testsuite/g++.old-deja/g++.mike/ns7.C index 57008db..7fd2b6f 100644 --- a/gcc/testsuite/g++.old-deja/g++.mike/ns7.C +++ b/gcc/testsuite/g++.old-deja/g++.mike/ns7.C @@ -6,4 +6,5 @@ namespace A { namespace B { int j = i; // { dg-error "" } + // { dg-message "A::i" "suggested alternative" { target *-*-* } 8 } } diff --git a/gcc/testsuite/g++.old-deja/g++.ns/koenig5.C b/gcc/testsuite/g++.old-deja/g++.ns/koenig5.C index 33061ad..43bcd37 100644 --- a/gcc/testsuite/g++.old-deja/g++.ns/koenig5.C +++ b/gcc/testsuite/g++.old-deja/g++.ns/koenig5.C @@ -15,4 +15,5 @@ void g() // foo variable first, and therefore do not // perform argument-dependent lookup. bar(new X); // { dg-error "not declared" } + // { dg-message "A::bar" "suggested alternative" { target *-*-* } 17 } } diff --git a/gcc/testsuite/g++.old-deja/g++.ns/koenig9.C b/gcc/testsuite/g++.old-deja/g++.ns/koenig9.C index 78b0e8b..0515217 100644 --- a/gcc/testsuite/g++.old-deja/g++.ns/koenig9.C +++ b/gcc/testsuite/g++.old-deja/g++.ns/koenig9.C @@ -10,4 +10,5 @@ void foo(const char*,...); inline void bar() { foo("",count); // { dg-error "" } multiple overloaded count functions + // { dg-message "std::count" "suggested alternative" { target *-*-* } 12 } } diff --git a/gcc/testsuite/g++.old-deja/g++.other/lineno5.C b/gcc/testsuite/g++.old-deja/g++.other/lineno5.C index d14bd90..11c8d9f 100644 --- a/gcc/testsuite/g++.old-deja/g++.other/lineno5.C +++ b/gcc/testsuite/g++.old-deja/g++.other/lineno5.C @@ -16,4 +16,5 @@ namespace tmp { class A { public: int kaka(tmp::B = b); // { dg-error "" } no b in scope + // { dg-message "tmp::b" "suggested alternative" { target *-*-* } 18 } };