public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [PATCH,c++] fix PR 45330, suggest alternatives for failed name lookups
@ 2010-11-10 18:20 Nathan Froyd
  2010-11-17  3:52 ` Mark Mitchell
  0 siblings, 1 reply; 19+ messages in thread
From: Nathan Froyd @ 2010-11-10 18:20 UTC (permalink / raw)
  To: gcc-patches; +Cc: jason

The testcase in PR 45330 is:

namespace N
{
 int foo;
}

int bar()
{
  return foo;
}

and the perceived problem is that GCC should suggest `N::foo' for the
unqualified `foo' in `bar'.  The below patch does just that: when name
lookup fails, we grovel through all known namespaces for potential
matches and suggest them to the user.

I put the declaration for the function in cp-tree.h rather than
name-lookup.h because it seemed like a better place than including
name-lookup.h in lex.c.  I'm happy to move the declaration to
name-lookup.h and add the include.  (Or, more radically, move
unqualified_name_lookup_error and unqualified_fn_lookup_error to
name-lookup.c and adjust includes appropriately.)

The error.c change is to avoid dump_expr complaining (as occurs in
g++.dg/lookup/koenig5.C).

I didn't include a testcase, as the testcase tweaks indicated there are
a number of testcases testing similar things.
(g++.old-deja/g++.mike/ns5.C is almost identical to the testcase in the
PR, for instance).

Tested on x86_64-unknown-linux-gnu.  OK to commit?

-Nathan

gcc/cp/
	PR c++/45330
	* cp-tree.h (suggest_alternatives_for): Declare.
	* error.c (dump_expr): Handle TYPE_DECL.
	* lex.c (unqualified_name_lookup_error): Call it.
	* name-lookup.c (suggest_alternatives_for_1): New function.
	(suggest_alternatives_for): New function.

gcc/testsuite/
	PR c++/45330
	* g++.dg/ext/builtin3.C: Adjust.
	* g++.dg/lookup/error1.C: Adjust.
	* g++.dg/lookup/koenig5.C: Adjust.
	* g++.dg/overload/koenig1.C: Adjust.
	* g++.dg/parse/decl-specifier-1.C: Adjust.
	* g++.dg/template/static10.C: Adjust.
	* g++.old-deja/g++.mike/ns5.C: Adjust.
	* g++.old-deja/g++.mike/ns7.C: Adjust.
	* g++.old-deja/g++.ns/koenig5.C: Adjust.
	* g++.old-deja/g++.ns/koenig9.C: Adjust.
	* g++.old-deja/g++.other/lineno5.C: Adjust.

diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
index 67f4f93..a323501 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/lex.c b/gcc/cp/lex.c
index c583d7d..9c6be41 100644
--- a/gcc/cp/lex.c
+++ b/gcc/cp/lex.c
@@ -450,7 +450,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/error.c b/gcc/cp/error.c
index 1560fc6..de45efe 100644
--- a/gcc/cp/error.c
+++ b/gcc/cp/error.c
@@ -1700,6 +1700,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/name-lookup.c b/gcc/cp/name-lookup.c
index 0a93da8..1f2f4e5 100644
--- a/gcc/cp/name-lookup.c
+++ b/gcc/cp/name-lookup.c
@@ -30,6 +30,7 @@ along with GCC; see the file COPYING3.  If not see
 #include "timevar.h"
 #include "toplev.h"
 #include "diagnostic-core.h"
+#include "intl.h"
 #include "debug.h"
 #include "c-family/c-pragma.h"
 
@@ -3919,6 +3920,62 @@ remove_hidden_names (tree fns)
   return fns;
 }
 
+/* Helper function for suggest_alternatives_for: lookup NAME in
+   namespace SCOPE and record any matches in CANDIDATES.  */
+
+static VEC(tree,heap) *
+suggest_alternatives_for_1 (tree name, tree scope,
+			    VEC(tree,heap) *candidates)
+{
+  struct scope_binding binding = EMPTY_SCOPE_BINDING;
+  struct cp_binding_level *level = NAMESPACE_LEVEL (scope);
+  tree t;
+
+  /* Look in this namespace.  */
+  qualified_lookup_using_namespace (name, scope, &binding, 0);
+
+  if (binding.value)
+    VEC_safe_push (tree, heap, candidates, binding.value);
+
+  /* Lookup in child namespaces.  */
+  for (t = level->namespaces; t; t = DECL_CHAIN (t))
+    candidates = suggest_alternatives_for_1 (name, t, candidates);
+
+  return candidates;
+}
+
+/* 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
+    = suggest_alternatives_for_1 (name, global_namespace, NULL);
+  const char *str;
+  char *spaces;
+  tree t;
+  unsigned ix;
+
+  /* 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);
+}
+
 /* Unscoped lookup of a global: iterate over current namespaces,
    considering using-directives.  */
 
diff --git a/gcc/testsuite/g++.dg/ext/builtin3.C b/gcc/testsuite/g++.dg/ext/builtin3.C
index 3d06dd7..a9ebce0 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 "note" "suggested alternative" { target *-*-* } 12 }
 }
diff --git a/gcc/testsuite/g++.dg/lookup/error1.C b/gcc/testsuite/g++.dg/lookup/error1.C
index 2264b23..3b34ee3 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 "note" "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..bc1dc8c 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 "note" "suggested alternative" { target *-*-* } 34 }
 
   Two (c); // ok
   Two (a, c); // ok
   Two (a); // { dg-error "not declared" }
+  // { dg-message "note" "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 "note" "suggested alternative" { target *-*-* } 47 }
 }
diff --git a/gcc/testsuite/g++.dg/overload/koenig1.C b/gcc/testsuite/g++.dg/overload/koenig1.C
index 1ed7bce..be0be69 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 "note" "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..48fc7fa 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 "note" "suggested alternative" { target *-*-* } 15 }
 }
diff --git a/gcc/testsuite/g++.dg/template/static10.C b/gcc/testsuite/g++.dg/template/static10.C
index ab857bd..2f60f0c 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 "note" "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..f13da04 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 "note" "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..31a71dd 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 "note" "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..67b781d 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 "note" "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..f246639 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 "note" "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..20e49bc 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 "note" "suggested alternative" { target *-*-* } 18 }
 };

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH,c++] fix PR 45330, suggest alternatives for failed name lookups
  2010-11-10 18:20 [PATCH,c++] fix PR 45330, suggest alternatives for failed name lookups Nathan Froyd
@ 2010-11-17  3:52 ` Mark Mitchell
  2010-12-02 19:11   ` Nathan Froyd
  0 siblings, 1 reply; 19+ messages in thread
From: Mark Mitchell @ 2010-11-17  3:52 UTC (permalink / raw)
  To: Nathan Froyd; +Cc: gcc-patches, jason

On 11/10/2010 10:13 AM, Nathan Froyd wrote:

> and the perceived problem is that GCC should suggest `N::foo' for the
> unqualified `foo' in `bar'.  The below patch does just that: when name
> lookup fails, we grovel through all known namespaces for potential
> matches and suggest them to the user.

I thought about this for a while from two angles:

(a) Will that be incredibly slow?
(b) Is there any reasonable way to limit the set of namespaces searched?

I decided (a) doesn't really matter; we're issuing an error, worst-case
we probably have a few thousand namespaces, should be OK.

For (b), the obvious thing would be to limit to parents and children
(but not siblings) of current namespaces.  So, for:

  namespace A {
    namespace B { }
    namespace C {
      namespace D { }
    }
  }

if the error is inside C, look in D, but not B.  But, that's probably
dumb; I don't really believe that this is more likely than B.  So, it
would only be a useful heuristic if we decided we really need to prune
the search.  And, of course, we could refuse to search more than 100
namespaces, if we wanted.  I think you should probably add a --param
(max-namespace-for-diagnostic-help) for that, and start with 1000 as the
default, just to ensure sanity?

> I didn't include a testcase, as the testcase tweaks indicated there are
> a number of testcases testing similar things.

OK, but can you make at least one of the testcases check for the proper
alternative name?  (Right now, it looks like you're just checking the
line numbers.)

Thank you,

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH,c++] fix PR 45330, suggest alternatives for failed name lookups
  2010-11-17  3:52 ` Mark Mitchell
@ 2010-12-02 19:11   ` Nathan Froyd
  2010-12-03 10:23     ` Dodji Seketeli
  0 siblings, 1 reply; 19+ messages in thread
From: Nathan Froyd @ 2010-12-02 19:11 UTC (permalink / raw)
  To: Mark Mitchell; +Cc: gcc-patches, jason

On Tue, Nov 16, 2010 at 06:31:19PM -0800, Mark Mitchell wrote:
> On 11/10/2010 10:13 AM, Nathan Froyd wrote:
> I thought about this for a while from two angles:
> 
> (a) Will that be incredibly slow?
> (b) Is there any reasonable way to limit the set of namespaces searched?
> 
> I decided (a) doesn't really matter; we're issuing an error, worst-case
> we probably have a few thousand namespaces, should be OK.

Yeah, I was worried about this too, but decided it didn't matter.

> And, of course, we could refuse to search more than 100 namespaces, if
> we wanted.  I think you should probably add a --param
> (max-namespace-for-diagnostic-help) for that, and start with 1000 as
> the default, just to ensure sanity?

New param added, cxx-max-namespaces-for-diagnostic-help.  Making sure to
consult this meant that it was somewhat easier to turn the suggestion
code into an iterative process rather than a recursive process, so I did
that.

> > I didn't include a testcase, as the testcase tweaks indicated there are
> > a number of testcases testing similar things.
> 
> OK, but can you make at least one of the testcases check for the proper
> alternative name?  (Right now, it looks like you're just checking the
> line numbers.)

I went ahead and updated all the testcases to check for the names
(particularly good for those tests where we search multiple
namespaces).  I also added a new testcase to make sure we handle the new
param correctly.

Tested on x86_64-linux-gnu.  OK to commit?

-Nathan

gcc/
	PR c++/45330
	* params.def (CXX_MAX_NAMESPACES_FOR_DIAGNOSTIC_HELP): New parameter.
	* doc/invoke.texi (cxx-max-namespaces-for-diagnostic-help): Document.

gcc/cp/
	PR c++/45330
	* cp-tree.h (suggest_alternatives_for): Declare.
	* error.c (dump_expr): Handle TYPE_DECL.
	* name-lookup.c (suggest_alternatives_for): New function.
	* lex.c (unqualified_name_lookup_error): Call it.

gcc/testsuite/
	PR c++/45330
	* g++.dg/pr45330.C: New test.
	* g++.dg/ext/builtin3.C: Adjust.
	* g++.dg/lookup/error1.C: Adjust.
	* g++.dg/lookup/koenig5.C: Adjust.
	* g++.dg/overload/koenig1.C: Adjust.
	* g++.dg/parse/decl-specifier-1.C: Adjust.
	* g++.dg/template/static10.C: Adjust.
	* g++.old-deja/g++.mike/ns5.C: Adjust.
	* g++.old-deja/g++.mike/ns7.C: Adjust.
	* g++.old-deja/g++.ns/koenig5.C: Adjust.
	* g++.old-deja/g++.ns/koenig9.C: Adjust.
	* g++.old-deja/g++.other/lineno5.C: Adjust.

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 }
 };

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH,c++] fix PR 45330, suggest alternatives for failed name lookups
  2010-12-02 19:11   ` Nathan Froyd
@ 2010-12-03 10:23     ` Dodji Seketeli
  2010-12-03 16:41       ` Nathan Froyd
  0 siblings, 1 reply; 19+ messages in thread
From: Dodji Seketeli @ 2010-12-03 10:23 UTC (permalink / raw)
  To: Nathan Froyd; +Cc: Mark Mitchell, gcc-patches, jason

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.

-- 
	Dodji

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH,c++] fix PR 45330, suggest alternatives for failed name lookups
  2010-12-03 10:23     ` Dodji Seketeli
@ 2010-12-03 16:41       ` Nathan Froyd
  2010-12-03 16:48         ` Gabriel Dos Reis
                           ` (3 more replies)
  0 siblings, 4 replies; 19+ messages in thread
From: Nathan Froyd @ 2010-12-03 16:41 UTC (permalink / raw)
  To: Dodji Seketeli; +Cc: Mark Mitchell, gcc-patches, jason

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH,c++] fix PR 45330, suggest alternatives for failed name lookups
  2010-12-03 16:41       ` Nathan Froyd
@ 2010-12-03 16:48         ` Gabriel Dos Reis
  2010-12-03 18:47           ` Nathan Froyd
  2010-12-04 14:06         ` Dodji Seketeli
                           ` (2 subsequent siblings)
  3 siblings, 1 reply; 19+ messages in thread
From: Gabriel Dos Reis @ 2010-12-03 16:48 UTC (permalink / raw)
  To: Nathan Froyd; +Cc: Dodji Seketeli, Mark Mitchell, gcc-patches, jason

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
>

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH,c++] fix PR 45330, suggest alternatives for failed name lookups
  2010-12-03 16:48         ` Gabriel Dos Reis
@ 2010-12-03 18:47           ` Nathan Froyd
  0 siblings, 0 replies; 19+ messages in thread
From: Nathan Froyd @ 2010-12-03 18:47 UTC (permalink / raw)
  To: Gabriel Dos Reis; +Cc: Dodji Seketeli, Mark Mitchell, gcc-patches, jason

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH,c++] fix PR 45330, suggest alternatives for failed name lookups
  2010-12-03 16:41       ` Nathan Froyd
  2010-12-03 16:48         ` Gabriel Dos Reis
@ 2010-12-04 14:06         ` Dodji Seketeli
  2010-12-04 23:16         ` Jason Merrill
  2010-12-05 16:09         ` Mark Mitchell
  3 siblings, 0 replies; 19+ messages in thread
From: Dodji Seketeli @ 2010-12-04 14:06 UTC (permalink / raw)
  To: Nathan Froyd; +Cc: Mark Mitchell, gcc-patches, jason

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 :-)

-- 
	Dodji

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH,c++] fix PR 45330, suggest alternatives for failed name lookups
  2010-12-03 16:41       ` Nathan Froyd
  2010-12-03 16:48         ` Gabriel Dos Reis
  2010-12-04 14:06         ` Dodji Seketeli
@ 2010-12-04 23:16         ` Jason Merrill
  2010-12-05 16:08           ` Mark Mitchell
  2010-12-05 16:09         ` Mark Mitchell
  3 siblings, 1 reply; 19+ messages in thread
From: Jason Merrill @ 2010-12-04 23:16 UTC (permalink / raw)
  To: Nathan Froyd; +Cc: Dodji Seketeli, Mark Mitchell, gcc-patches

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH,c++] fix PR 45330, suggest alternatives for failed name lookups
  2010-12-04 23:16         ` Jason Merrill
@ 2010-12-05 16:08           ` Mark Mitchell
  2010-12-06 19:00             ` Nathan Froyd
  0 siblings, 1 reply; 19+ messages in thread
From: Mark Mitchell @ 2010-12-05 16:08 UTC (permalink / raw)
  To: Jason Merrill; +Cc: Nathan Froyd, Dodji Seketeli, gcc-patches

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).

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH,c++] fix PR 45330, suggest alternatives for failed name lookups
  2010-12-03 16:41       ` Nathan Froyd
                           ` (2 preceding siblings ...)
  2010-12-04 23:16         ` Jason Merrill
@ 2010-12-05 16:09         ` Mark Mitchell
  2010-12-06 20:26           ` Nathan Froyd
  3 siblings, 1 reply; 19+ messages in thread
From: Mark Mitchell @ 2010-12-05 16:09 UTC (permalink / raw)
  To: Nathan Froyd; +Cc: Dodji Seketeli, gcc-patches, jason

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,

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH,c++] fix PR 45330, suggest alternatives for failed name lookups
  2010-12-05 16:08           ` Mark Mitchell
@ 2010-12-06 19:00             ` Nathan Froyd
  2010-12-06 19:05               ` Jason Merrill
  0 siblings, 1 reply; 19+ messages in thread
From: Nathan Froyd @ 2010-12-06 19:00 UTC (permalink / raw)
  To: Mark Mitchell; +Cc: Jason Merrill, Dodji Seketeli, gcc-patches

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH,c++] fix PR 45330, suggest alternatives for failed name lookups
  2010-12-06 19:00             ` Nathan Froyd
@ 2010-12-06 19:05               ` Jason Merrill
  2010-12-06 19:10                 ` Gabriel Dos Reis
  0 siblings, 1 reply; 19+ messages in thread
From: Jason Merrill @ 2010-12-06 19:05 UTC (permalink / raw)
  To: Nathan Froyd; +Cc: Mark Mitchell, Dodji Seketeli, gcc-patches

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH,c++] fix PR 45330, suggest alternatives for failed name lookups
  2010-12-06 19:05               ` Jason Merrill
@ 2010-12-06 19:10                 ` Gabriel Dos Reis
  0 siblings, 0 replies; 19+ messages in thread
From: Gabriel Dos Reis @ 2010-12-06 19:10 UTC (permalink / raw)
  To: Jason Merrill; +Cc: Nathan Froyd, Mark Mitchell, Dodji Seketeli, gcc-patches

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH,c++] fix PR 45330, suggest alternatives for failed name lookups
  2010-12-05 16:09         ` Mark Mitchell
@ 2010-12-06 20:26           ` Nathan Froyd
  2010-12-06 22:28             ` Gabriel Dos Reis
  2010-12-07  5:47             ` Mark Mitchell
  0 siblings, 2 replies; 19+ messages in thread
From: Nathan Froyd @ 2010-12-06 20:26 UTC (permalink / raw)
  To: Mark Mitchell; +Cc: Dodji Seketeli, gcc-patches, jason

On Sun, Dec 05, 2010 at 10:09:09AM -0600, Mark Mitchell wrote:
> 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.
> 
> > 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.

I tried making Dodji's changes (though I grab the location from the tree
passed to suggest_alternatives, rather than passing the location in from
above), which prompted me to make a couple more:

- We should really be using inform_n, rather than some hacky
  format-already-translated-strings-into-messages scheme;

- We shouldn't tell the user that we hit the max namespace limit if
  there aren't any more namespaces to search.

- When suggesting alternatives, we should indicate the location of the
  alternative we're suggesting rather than the location of the name we
  failed to lookup.

  That is, for, say, g++.old-deja/g++.mike/ns5.C:

// { dg-do assemble  }
namespace A {
  int i = 1;
}

int j = i;

  We should output:

ns5.C:6:9: error: 'i' was not declared in this scope
ns5.C:6:9: note: suggested alternative:
ns5.C:3:7: note:   'A::i'

  instead of:

ns5.C:6:9: error: 'i' was not declared in this scope
ns5.C:6:9: note: suggested alternative:
ns5.C:6:9: note:   'A::i'

I think these three changes are non-controversial.  Please let me know
if you feel differently.

However, this causes a problem for g++.old-deja/g++.other/koenig9.C,
since we now report a location in system headers and AFAICS, there's no
good way to handle that in DejaGNU.  Looking a little further, it
appears that the test actually wanted:

koenig9.C:13: error: overloaded function with no contextual type information

rather than a complaint about an undeclared name.  I guess at some point
in the past, <algorithm> automatically use'd std; now that it no longer
does this, we don't test the same thing we were testing before.  I have
changed the test to use locally-declared count functions, which I think
keeps the spirit of the test better.

OK with the above changes?

-Nathan

gcc/
	PR c++/45330
	* params.def (CXX_MAX_NAMESPACES_FOR_DIAGNOSTIC_HELP): New parameter.
	* doc/invoke.texi (cxx-max-namespaces-for-diagnostic-help): Document.

gcc/cp/
	PR c++/45330
	* cp-tree.h (suggest_alternatives_for, location_of): Declare.
	* error.c (dump_expr): Handle TYPE_DECL.
	(location_of): Unstaticize.
	* name-lookup.c (suggest_alternatives_for): New function.
	* lex.c (unqualified_name_lookup_error): Call it.

gcc/testsuite/
	PR c++/45330
	* g++.dg/pr45330.C: New test.
	* g++.dg/ext/builtin3.C: Adjust.
	* g++.dg/lookup/error1.C: Adjust.
	* g++.dg/lookup/koenig5.C: Adjust.
	* g++.dg/overload/koenig1.C: Adjust.
	* g++.dg/parse/decl-specifier-1.C: Adjust.
	* g++.dg/template/static10.C: Adjust.
	* g++.old-deja/g++.mike/ns5.C: Adjust.
	* g++.old-deja/g++.mike/ns7.C: Adjust.
	* g++.old-deja/g++.ns/koenig5.C: Adjust.
	* g++.old-deja/g++.ns/koenig9.C: Adjust.
	* g++.old-deja/g++.other/lineno5.C: Adjust.

diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
index 59342e3..aba8dfd 100644
--- a/gcc/cp/cp-tree.h
+++ b/gcc/cp/cp-tree.h
@@ -4907,6 +4907,7 @@ extern void print_instantiation_context		(void);
 extern void maybe_warn_variadic_templates       (void);
 extern void maybe_warn_cpp0x			(cpp0x_warn_str str);
 extern bool pedwarn_cxx98                       (location_t, int, const char *, ...) ATTRIBUTE_GCC_DIAG(3,4);
+extern location_t location_of                   (tree);
 
 /* in except.c */
 extern void init_exception_processing		(void);
@@ -5639,6 +5640,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 ed168c4..4fb47dc 100644
--- a/gcc/cp/error.c
+++ b/gcc/cp/error.c
@@ -98,7 +98,6 @@ static void cp_print_error_function (diagnostic_context *, diagnostic_info *);
 
 static bool cp_printer (pretty_printer *, text_info *, const char *,
 			int, bool, bool, bool);
-static location_t location_of (tree);
 
 void
 init_error (void)
@@ -1700,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;
@@ -2487,7 +2487,7 @@ lang_decl_name (tree decl, int v, bool translate)
 
 /* Return the location of a tree passed to %+ formats.  */
 
-static location_t
+location_t
 location_of (tree t)
 {
   if (TREE_CODE (t) == PARM_DECL && DECL_CONTEXT (t))
diff --git a/gcc/cp/lex.c b/gcc/cp/lex.c
index 684803f..5a2ae41 100644
--- a/gcc/cp/lex.c
+++ b/gcc/cp/lex.c
@@ -450,7 +450,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..4cf1380 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,71 @@ 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;
+  tree t;
+  unsigned ix;
+  location_t name_location;
+
+  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);
+    }
+
+  name_location = location_of (name);
+
+  /* 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
+      && !VEC_empty (tree, namespaces_to_search))
+    inform (name_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;
+
+  inform_n (name_location, VEC_length (tree, candidates),
+	    "suggested alternative:",
+	    "suggested alternatives:");
+
+  FOR_EACH_VEC_ELT (tree, candidates, ix, t)
+    inform (location_of (t), "  %qE", t);
+
+  VEC_free (tree, heap, candidates);
+}
+
 /* 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 2813532..4e3d002 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -8821,6 +8821,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..001d5f7 100644
--- a/gcc/testsuite/g++.dg/ext/builtin3.C
+++ b/gcc/testsuite/g++.dg/ext/builtin3.C
@@ -5,9 +5,10 @@
 // { dg-options "" }
 
 namespace std {
-extern "C" int printf(char*, ...);
+extern "C" int printf(char*, ...); // { dg-message "std::printf" }
 }
 
 void foo() {
   printf("abc"); 		// { dg-error "not declared" }
+  // { dg-message "suggested alternative" "suggested alternative" { target *-*-* } 12 }
 }
diff --git a/gcc/testsuite/g++.dg/lookup/error1.C b/gcc/testsuite/g++.dg/lookup/error1.C
index 2264b23..3eb4b97 100644
--- a/gcc/testsuite/g++.dg/lookup/error1.C
+++ b/gcc/testsuite/g++.dg/lookup/error1.C
@@ -2,8 +2,9 @@
 // Origin: <papadopo@shfj.cea.fr>
 // { dg-do compile }
 
-namespace N { int i; }
+namespace N { int i; }		// { dg-message "N::i" }
 void foo() { i; }   // { dg-error "not declared" }
+  // { dg-message "suggested alternative" "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..c44543b 100644
--- a/gcc/testsuite/g++.dg/lookup/koenig5.C
+++ b/gcc/testsuite/g++.dg/lookup/koenig5.C
@@ -8,23 +8,23 @@
 namespace N
 {
   struct A {};
-  void One (...);
-  void (*Two) (...);
-  namespace Three {}
+  void One (...);		// { dg-message "N::One" }
+  void (*Two) (...);		// { dg-message "N::Two" }
+  namespace Three {}		// { dg-message "N::Three" }
 }
 
 namespace M
 {
   struct B {};
-  struct One {};
-  void (*Two) (...);
-  void Three (...);
+  struct One {};		// { dg-message "M::One" }
+  void (*Two) (...);		// { dg-message "M::Two" }
+  void Three (...);		// { dg-message "M::Three" }
 }
 
 namespace O 
 {
   struct C {};
-  void Two (...);
+  void Two (...);		// { dg-message "O::Two" }
 }
   
 void g (N::A *a, M::B *b, O::C *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 "suggested alternatives" "suggested alternative for One" { target *-*-* } 34 }
 
   Two (c); // ok
   Two (a, c); // ok
   Two (a); // { dg-error "not declared" }
+  // { dg-message "suggested alternatives" "suggested alternative for Two" { 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 "suggested alternatives" "suggested alternative for Three" { target *-*-* } 47 }
 }
diff --git a/gcc/testsuite/g++.dg/overload/koenig1.C b/gcc/testsuite/g++.dg/overload/koenig1.C
index 1ed7bce..a461259 100644
--- a/gcc/testsuite/g++.dg/overload/koenig1.C
+++ b/gcc/testsuite/g++.dg/overload/koenig1.C
@@ -3,7 +3,7 @@
 // valid call.
 
 namespace N {
-  template <class T> void f (T);
+  template <class T> void f (T); // { dg-message "N::f" }
   struct A;
 }
 
@@ -14,5 +14,6 @@ void g ()
   B *bp;
   N::A *ap;
   f (bp);			// { dg-error "not declared" }
+  // { dg-message "suggested alternative" "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..baf0fe7 100644
--- a/gcc/testsuite/g++.dg/parse/decl-specifier-1.C
+++ b/gcc/testsuite/g++.dg/parse/decl-specifier-1.C
@@ -5,7 +5,7 @@
 namespace N
 {
     template<typename> 
-    struct X { };
+    struct X { };		// { dg-message "N::X" }
 }
 
 N::X X;                           // { dg-error "" "" }
@@ -13,4 +13,5 @@ N::X X;                           // { dg-error "" "" }
 int main()
 {
     return sizeof(X);             // { dg-error "" "" }
+    // { dg-message "suggested alternative" "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..02d9b3f
--- /dev/null
+++ b/gcc/testsuite/g++.dg/pr45330.C
@@ -0,0 +1,37 @@
+// { 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; }
+
+namespace A
+{
+  int foo;			// { dg-message "A::foo" "suggested alternative" }
+}
+
+namespace B
+{
+  int foo;
+}
+
+namespace C
+{
+  int foo;
+}
+
+namespace D
+{
+  int foo;
+}
+
+namespace E
+{
+  int foo;
+}
+
+int bar()
+{
+  return foo;			// { dg-error "was not declared" }
+  // { dg-message "maximum limit of 4 namespaces" "maximum limit" { target *-*-* } 34 }
+  // { dg-message "suggested alternative" "suggested alternative" { target *-*-* } 34 }
+}
diff --git a/gcc/testsuite/g++.dg/template/static10.C b/gcc/testsuite/g++.dg/template/static10.C
index ab857bd..881db08 100644
--- a/gcc/testsuite/g++.dg/template/static10.C
+++ b/gcc/testsuite/g++.dg/template/static10.C
@@ -4,7 +4,7 @@ namespace __gnu_debug_def { }
 namespace std
 {
   using namespace __gnu_debug_def;
-  template<typename _Tp> class allocator {};
+  template<typename _Tp> class allocator {}; // { dg-message "std::allocator" }
 }
 namespace __gnu_debug_def
 {
@@ -20,4 +20,5 @@ namespace std
 {
   template<> void
   vector<int, allocator<int> >::swap(vector<int, allocator<int> >&) { } // { dg-error "" }
+  // { dg-message "suggested alternative" "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..fd1fbff 100644
--- a/gcc/testsuite/g++.old-deja/g++.mike/ns5.C
+++ b/gcc/testsuite/g++.old-deja/g++.mike/ns5.C
@@ -1,6 +1,7 @@
 // { dg-do assemble  }
 namespace A {
-  int i = 1;
+  int i = 1;			// { dg-message "A::i" }
 }
 
 int j = i;		// { dg-error "" } 
+  // { dg-message "suggested alternative" "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..67d9e77 100644
--- a/gcc/testsuite/g++.old-deja/g++.mike/ns7.C
+++ b/gcc/testsuite/g++.old-deja/g++.mike/ns7.C
@@ -1,9 +1,10 @@
 // { dg-do assemble  }
 
 namespace A {
-  int i = 1;
+  int i = 1;			// { dg-message "A::i" }
 }
 
 namespace B {
   int j = i;	// { dg-error "" } 
+  // { dg-message "suggested alternative" "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..7c56d5c 100644
--- a/gcc/testsuite/g++.old-deja/g++.ns/koenig5.C
+++ b/gcc/testsuite/g++.old-deja/g++.ns/koenig5.C
@@ -3,7 +3,7 @@
 namespace A{
   void foo();             
   struct X{};
-  void (*bar)(X*)=0;
+  void (*bar)(X*)=0;		// { dg-message "A::bar" }
 }
 using A::X;
 
@@ -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 "suggested alternative" "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..46efcb7 100644
--- a/gcc/testsuite/g++.old-deja/g++.ns/koenig9.C
+++ b/gcc/testsuite/g++.old-deja/g++.ns/koenig9.C
@@ -3,11 +3,12 @@
 // Copyright (C) 2000 Free Software Foundation, Inc.
 // Contributed by Theodore.Papadopoulo 23 Jun 2000 <Theodore.Papadopoulo@sophia.inria.fr>
 
-#include <algorithm>
+int count (int);
+void *count (char *, char);
 
 void foo(const char*,...);
 
 inline void
 bar() {
-  foo("",count);    //  { dg-error "" } multiple overloaded count functions
+  foo("",count);    //  { dg-error "overloaded function" "multiple overloaded functions" }
 }
diff --git a/gcc/testsuite/g++.old-deja/g++.other/lineno5.C b/gcc/testsuite/g++.old-deja/g++.other/lineno5.C
index d14bd90..d227339 100644
--- a/gcc/testsuite/g++.old-deja/g++.other/lineno5.C
+++ b/gcc/testsuite/g++.old-deja/g++.other/lineno5.C
@@ -10,10 +10,11 @@
 
 namespace tmp {
   typedef int B;
-  B b;
+  B b;				// { dg-message "tmp::b" }
 }
 
 class A {
   public:
   int kaka(tmp::B = b);		// { dg-error "" } no b in scope
+  // { dg-message "suggested alternative" "suggested alternative" { target *-*-* } 18 }
 };

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH,c++] fix PR 45330, suggest alternatives for failed name lookups
  2010-12-06 20:26           ` Nathan Froyd
@ 2010-12-06 22:28             ` Gabriel Dos Reis
  2010-12-07  6:05               ` Mark Mitchell
  2010-12-07  5:47             ` Mark Mitchell
  1 sibling, 1 reply; 19+ messages in thread
From: Gabriel Dos Reis @ 2010-12-06 22:28 UTC (permalink / raw)
  To: Nathan Froyd; +Cc: Mark Mitchell, Dodji Seketeli, gcc-patches, jason

On Mon, Dec 6, 2010 at 2:23 PM, Nathan Froyd <froydnj@codesourcery.com> wrote:
> On Sun, Dec 05, 2010 at 10:09:09AM -0600, Mark Mitchell wrote:
>> 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.
>>
>> > 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.
>
> I tried making Dodji's changes (though I grab the location from the tree
> passed to suggest_alternatives, rather than passing the location in from
> above), which prompted me to make a couple more:
>
> - We should really be using inform_n, rather than some hacky
>  format-already-translated-strings-into-messages scheme;
>
> - We shouldn't tell the user that we hit the max namespace limit if
>  there aren't any more namespaces to search.
>
> - When suggesting alternatives, we should indicate the location of the
>  alternative we're suggesting rather than the location of the name we
>  failed to lookup.
>
>  That is, for, say, g++.old-deja/g++.mike/ns5.C:
>
> // { dg-do assemble  }
> namespace A {
>  int i = 1;
> }
>
> int j = i;
>
>  We should output:
>
> ns5.C:6:9: error: 'i' was not declared in this scope
> ns5.C:6:9: note: suggested alternative:
> ns5.C:3:7: note:   'A::i'
>
>  instead of:
>
> ns5.C:6:9: error: 'i' was not declared in this scope
> ns5.C:6:9: note: suggested alternative:
> ns5.C:6:9: note:   'A::i'
>
> I think these three changes are non-controversial.  Please let me know
> if you feel differently.
>
> However, this causes a problem for g++.old-deja/g++.other/koenig9.C,
> since we now report a location in system headers and AFAICS, there's no
> good way to handle that in DejaGNU.  Looking a little further, it
> appears that the test actually wanted:
>
> koenig9.C:13: error: overloaded function with no contextual type information
>
> rather than a complaint about an undeclared name.  I guess at some point
> in the past, <algorithm> automatically use'd std; now that it no longer
> does this, we don't test the same thing we were testing before.  I have
> changed the test to use locally-declared count functions, which I think
> keeps the spirit of the test better.

I think we should treat names from system headers as abstract (e.g. print
only the standad name of the header) as opposed to
dumping their physical locations in the diagnostics (unless explicitly
requested.)

-- Gaby

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH,c++] fix PR 45330, suggest alternatives for failed name lookups
  2010-12-06 20:26           ` Nathan Froyd
  2010-12-06 22:28             ` Gabriel Dos Reis
@ 2010-12-07  5:47             ` Mark Mitchell
  1 sibling, 0 replies; 19+ messages in thread
From: Mark Mitchell @ 2010-12-07  5:47 UTC (permalink / raw)
  To: Nathan Froyd; +Cc: Dodji Seketeli, gcc-patches, jason

On 12/6/2010 12:23 PM, Nathan Froyd wrote:

> - We should really be using inform_n, rather than some hacky
>   format-already-translated-strings-into-messages scheme;

Yes.

> - We shouldn't tell the user that we hit the max namespace limit if
>   there aren't any more namespaces to search.

Yes.

> - When suggesting alternatives, we should indicate the location of the
>   alternative we're suggesting rather than the location of the name we
>   failed to lookup.

Yes.

> OK with the above changes?

Yes.

Thank you,

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH,c++] fix PR 45330, suggest alternatives for failed name lookups
  2010-12-06 22:28             ` Gabriel Dos Reis
@ 2010-12-07  6:05               ` Mark Mitchell
  2010-12-07 14:59                 ` Nathan Froyd
  0 siblings, 1 reply; 19+ messages in thread
From: Mark Mitchell @ 2010-12-07  6:05 UTC (permalink / raw)
  To: Gabriel Dos Reis; +Cc: Nathan Froyd, Dodji Seketeli, gcc-patches, jason

On 12/6/2010 2:23 PM, Gabriel Dos Reis wrote:

> I think we should treat names from system headers as abstract (e.g. print
> only the standad name of the header) as opposed to
> dumping their physical locations in the diagnostics (unless explicitly
> requested.)

I agree, saying <algorithm> or <memory> would be best.  But, for
avoidance of doubt, I think that enhancement is orthogonal to the one in
Nathan's patch.

Thank you,

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH,c++] fix PR 45330, suggest alternatives for failed name lookups
  2010-12-07  6:05               ` Mark Mitchell
@ 2010-12-07 14:59                 ` Nathan Froyd
  0 siblings, 0 replies; 19+ messages in thread
From: Nathan Froyd @ 2010-12-07 14:59 UTC (permalink / raw)
  To: Mark Mitchell; +Cc: Gabriel Dos Reis, Dodji Seketeli, gcc-patches, jason

On Mon, Dec 06, 2010 at 05:19:48PM -0800, Mark Mitchell wrote:
> On 12/6/2010 2:23 PM, Gabriel Dos Reis wrote:
> 
> > I think we should treat names from system headers as abstract (e.g. print
> > only the standad name of the header) as opposed to
> > dumping their physical locations in the diagnostics (unless explicitly
> > requested.)
> 
> I agree, saying <algorithm> or <memory> would be best.  But, for
> avoidance of doubt, I think that enhancement is orthogonal to the one in
> Nathan's patch.

FWIW, PR c++/46836 has been filed to record this suggestion.

-Nathan

^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2010-12-07 14:58 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-11-10 18:20 [PATCH,c++] fix PR 45330, suggest alternatives for failed name lookups Nathan Froyd
2010-11-17  3:52 ` Mark Mitchell
2010-12-02 19:11   ` Nathan Froyd
2010-12-03 10:23     ` Dodji Seketeli
2010-12-03 16:41       ` Nathan Froyd
2010-12-03 16:48         ` Gabriel Dos Reis
2010-12-03 18:47           ` Nathan Froyd
2010-12-04 14:06         ` Dodji Seketeli
2010-12-04 23:16         ` Jason Merrill
2010-12-05 16:08           ` Mark Mitchell
2010-12-06 19:00             ` Nathan Froyd
2010-12-06 19:05               ` Jason Merrill
2010-12-06 19:10                 ` Gabriel Dos Reis
2010-12-05 16:09         ` Mark Mitchell
2010-12-06 20:26           ` Nathan Froyd
2010-12-06 22:28             ` Gabriel Dos Reis
2010-12-07  6:05               ` Mark Mitchell
2010-12-07 14:59                 ` Nathan Froyd
2010-12-07  5:47             ` Mark Mitchell

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).