[C++ Patch] Fix PR 35112

[C++ Patch] Fix PR 35112

[Paolo Carlini]

[-- Attachment #1: Type: text/plain, Size: 849 bytes --]

Hi,

in this case the issue is slightly less simple than some other small
patches I prepared lately. The PR is about a problem while printing the
set of candidates for an ambiguous lookup, which I tracked down to
print_candidates no dealing correctly with TREE_VALUE of the argument as
TREE_LIST. Having fixed that, I noticed that the now correct diagnostics
was emitted twice, that was evidently a pre-existing problem along this
code path. Indeed parse/error17.C showed exactly the same problem; also
template/dtor7.C had some dg-bogus. Thus I figured out that apparently
in cp_parser_class_name we were emitting a second time the diagnostics
already emitted inside cp_parser_lookup_name and tried to clean-up that,
that is the work done right after the latter call.

Patch tested x86_64-linux. Is it ok?

Thanks,
Paolo.

////////////////////////

[-- Attachment #2: CL_35112 --]
[-- Type: text/plain, Size: 506 bytes --]

/cp
2009-11-27  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/35112
	* pt.c (print_candidates): Deal with TREE_LIST as TREE_VALUE of the
	argument.
	* parser.c (cp_parser_class_name): Do not duplicate the diagnostics
	after the cp_parser_lookup_name call.

/testsuite
2009-11-27  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/35112
	* g++.dg/parse/crash53.C: New.
	* g++.dg/parse/error17.C: Adjust, error messages are not
	duplicated anymore.
	* g++.dg/template/dtor7.C: Remove dg-bogus markers.

[-- Attachment #3: patch_35112 --]
[-- Type: text/plain, Size: 3468 bytes --]

Index: testsuite/g++.dg/parse/crash53.C
===================================================================
--- testsuite/g++.dg/parse/crash53.C	(revision 0)
+++ testsuite/g++.dg/parse/crash53.C	(revision 0)
@@ -0,0 +1,13 @@
+// PR c++/35112
+
+namespace X { struct A; }  // { dg-error "struct X::A" }
+namespace Y { struct A; }  // { dg-error "struct Y::A" }
+namespace Z { struct A; }  // { dg-error "struct Z::A" }
+namespace W { struct A; }  // { dg-error "struct W::A" }
+
+using namespace X;
+using namespace Y;
+using namespace Z;
+using namespace W;
+
+A* p; // { dg-error "reference to 'A' is ambiguous|'A' does not name a type" }
Index: testsuite/g++.dg/parse/error17.C
===================================================================
--- testsuite/g++.dg/parse/error17.C	(revision 154678)
+++ testsuite/g++.dg/parse/error17.C	(working copy)
@@ -2,10 +2,8 @@
 // PR c++/16965
 
 template <typename T> struct B { 
-  static int Bar(T); // { dg-error "14:candidates are: " "1" }
-  // { dg-error "14:with T = int" "2" { target *-*-* } 5 }
+  static int Bar(T); // { dg-error "14:candidates are: |with T = int" }
 }; 
 struct D : B<int>, B<char> {}; 
  
-int i2 = D::Bar(2); // { dg-error "13:reference to 'Bar' is ambiguous" }
-// { dg-error "10:reference to 'Bar' is ambiguous" "2" { target *-*-* } 10 }
+int i2 = D::Bar(2); // { dg-error "10:reference to 'Bar' is ambiguous" }
Index: testsuite/g++.dg/template/dtor7.C
===================================================================
--- testsuite/g++.dg/template/dtor7.C	(revision 154678)
+++ testsuite/g++.dg/template/dtor7.C	(working copy)
@@ -1,10 +1,10 @@
 // PR c++/40373
 // { dg-compile }
 
-struct A;	// { dg-bogus "candidates are" "" { xfail *-*-* } }
+struct A;
 namespace
 {
-  struct A;	// { dg-bogus "struct" "" { xfail *-*-* } }
+  struct A;
 }
 
 struct B {};
@@ -20,5 +20,3 @@ bar ()
 {
   foo (B ());	// { dg-bogus "instantiated from here" "" { xfail *-*-* } }
 }
-
-// { dg-bogus "is ambiguous" "" { xfail *-*-* } 15 }
Index: cp/pt.c
===================================================================
--- cp/pt.c	(revision 154684)
+++ cp/pt.c	(working copy)
@@ -1664,12 +1664,18 @@ print_candidates (tree fns)
 	  str = "               ";
 	}
     }
-  else for (fn = fns; fn != NULL_TREE; fn = TREE_CHAIN (fn))
-    {
-      for (f = TREE_VALUE (fn); f; f = OVL_NEXT (f))
-	error ("%s %+#D", str, OVL_CURRENT (f));
-      str = "               ";
-    }
+  else
+    for (fn = fns; fn != NULL_TREE; fn = TREE_CHAIN (fn))
+      {
+	for (f = TREE_VALUE (fn); f; f = OVL_NEXT (f))
+	  {
+	    if (TREE_CODE (f) == TREE_LIST)
+	      print_candidates (f);
+	    else
+	      error ("%s %+#D", str, OVL_CURRENT (f));
+	  }
+	str = "               ";
+      }
 }
 
 /* Returns the template (one of the functions given by TEMPLATE_ID)
Index: cp/parser.c
===================================================================
--- cp/parser.c	(revision 154684)
+++ cp/parser.c	(working copy)
@@ -15739,14 +15739,8 @@ cp_parser_class_name (cp_parser *parser,
 					identifier_token->location);
 	  if (ambiguous_decls)
 	    {
-	      error_at (identifier_token->location,
-			"reference to %qD is ambiguous", identifier);
-	      print_candidates (ambiguous_decls);
 	      if (cp_parser_parsing_tentatively (parser))
-		{
-		  identifier_token->ambiguous_p = true;
-		  cp_parser_simulate_error (parser);
-		}
+		cp_parser_simulate_error (parser);
 	      return error_mark_node;
 	    }
 	}

Re: [C++ Patch] Fix PR 35112

[Gabriel Dos Reis]

On Thu, Nov 26, 2009 at 7:31 PM, Paolo Carlini <paolo.carlini@oracle.com> wrote:
> Hi,
>
> in this case the issue is slightly less simple than some other small
> patches I prepared lately. The PR is about a problem while printing the
> set of candidates for an ambiguous lookup, which I tracked down to
> print_candidates no dealing correctly with TREE_VALUE of the argument as
> TREE_LIST. Having fixed that, I noticed that the now correct diagnostics
> was emitted twice, that was evidently a pre-existing problem along this
> code path. Indeed parse/error17.C showed exactly the same problem; also
> template/dtor7.C had some dg-bogus. Thus I figured out that apparently
> in cp_parser_class_name we were emitting a second time the diagnostics
> already emitted inside cp_parser_lookup_name and tried to clean-up that,
> that is the work done right after the latter call.
>
> Patch tested x86_64-linux. Is it ok?

Looks good to me.  Is there a chance you could put
the new code into a separate function of its own?

-- Gaby

Re: [C++ Patch] Fix PR 35112

[Paolo Carlini]

On 11/27/2009 02:42 AM, Gabriel Dos Reis wrote:
> Looks good to me.  Is there a chance you could put
> the new code into a separate function of its own?
>   
Sure! Are you willing to suggest an appropriate name? I don't want to
risk coming up with something not to your taste...

Paolo.

Re: [C++ Patch] Fix PR 35112

[Gabriel Dos Reis]

On Thu, Nov 26, 2009 at 7:44 PM, Paolo Carlini <paolo.carlini@oracle.com> wrote:
> On 11/27/2009 02:42 AM, Gabriel Dos Reis wrote:
>> Looks good to me.  Is there a chance you could put
>> the new code into a separate function of its own?
>>
> Sure! Are you willing to suggest an appropriate name? I don't want to
> risk coming up with something not to your taste...

Don't pretend I'm that difficult :-)

Seriously, here is what I meant:  in the current print_candidates
the inner for loop i(the one that goes over functions in an
overload set) s repeated twice.  So, you can make
that a function of its own, say print_overloaded_functions.
Then print_candidates calls print_overloaded_functions in
the is_overloaded_fn branch, but loops over the list
of overload set in the else branch, calling print_overloaded_functions
with the TREE_VALUE.  Does that make sense to you?

-- Gaby

Re: [C++ Patch] Fix PR 35112

[Paolo Carlini]

[-- Attachment #1: Type: text/plain, Size: 644 bytes --]

Hi again,
> Don't pretend I'm that difficult :-)
>
> Seriously, here is what I meant:  in the current print_candidates
> the inner for loop i(the one that goes over functions in an
> overload set) s repeated twice.  So, you can make
> that a function of its own, say print_overloaded_functions.
> Then print_candidates calls print_overloaded_functions in
> the is_overloaded_fn branch, but loops over the list
> of overload set in the else branch, calling print_overloaded_functions
> with the TREE_VALUE.  Does that make sense to you?
>   
Sure. The below is what I have tested on x86_64-linux. Is it ok?

Thanks,
Paolo.

////////////////////

[-- Attachment #2: CL_35112_2 --]
[-- Type: text/plain, Size: 519 bytes --]

/cp
2009-11-27  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/35112
	* pt.c (print_overloaded_functions): New.
	(print_candidates): Call the latter.
	* parser.c (cp_parser_class_name): Do not duplicate the diagnostics
	after the cp_parser_lookup_name call.

/testsuite
2009-11-27  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/35112
	* g++.dg/parse/crash53.C: New.
	* g++.dg/parse/error17.C: Adjust, error messages are not
	duplicated anymore.
	* g++.dg/template/dtor7.C: Remove xfail-ed dg-bogus directives.

[-- Attachment #3: patch_35112_2 --]
[-- Type: text/plain, Size: 4094 bytes --]

Index: testsuite/g++.dg/parse/crash53.C
===================================================================
--- testsuite/g++.dg/parse/crash53.C	(revision 0)
+++ testsuite/g++.dg/parse/crash53.C	(revision 0)
@@ -0,0 +1,13 @@
+// PR c++/35112
+
+namespace X { struct A; }  // { dg-error "struct X::A" }
+namespace Y { struct A; }  // { dg-error "struct Y::A" }
+namespace Z { struct A; }  // { dg-error "struct Z::A" }
+namespace W { struct A; }  // { dg-error "struct W::A" }
+
+using namespace X;
+using namespace Y;
+using namespace Z;
+using namespace W;
+
+A* p; // { dg-error "reference to 'A' is ambiguous|'A' does not name a type" }
Index: testsuite/g++.dg/parse/error17.C
===================================================================
--- testsuite/g++.dg/parse/error17.C	(revision 154697)
+++ testsuite/g++.dg/parse/error17.C	(working copy)
@@ -2,10 +2,8 @@
 // PR c++/16965
 
 template <typename T> struct B { 
-  static int Bar(T); // { dg-error "14:candidates are: " "1" }
-  // { dg-error "14:with T = int" "2" { target *-*-* } 5 }
+  static int Bar(T); // { dg-error "14:candidates are: |with T = int" }
 }; 
 struct D : B<int>, B<char> {}; 
  
-int i2 = D::Bar(2); // { dg-error "13:reference to 'Bar' is ambiguous" }
-// { dg-error "10:reference to 'Bar' is ambiguous" "2" { target *-*-* } 10 }
+int i2 = D::Bar(2); // { dg-error "10:reference to 'Bar' is ambiguous" }
Index: testsuite/g++.dg/template/dtor7.C
===================================================================
--- testsuite/g++.dg/template/dtor7.C	(revision 154697)
+++ testsuite/g++.dg/template/dtor7.C	(working copy)
@@ -1,10 +1,10 @@
 // PR c++/40373
 // { dg-compile }
 
-struct A;	// { dg-bogus "candidates are" "" { xfail *-*-* } }
+struct A;
 namespace
 {
-  struct A;	// { dg-bogus "struct" "" { xfail *-*-* } }
+  struct A;
 }
 
 struct B {};
@@ -20,5 +20,3 @@ bar ()
 {
   foo (B ());	// { dg-bogus "instantiated from here" "" { xfail *-*-* } }
 }
-
-// { dg-bogus "is ambiguous" "" { xfail *-*-* } 15 }
Index: cp/pt.c
===================================================================
--- cp/pt.c	(revision 154697)
+++ cp/pt.c	(working copy)
@@ -1646,30 +1646,37 @@ explicit_class_specialization_p (tree type)
   return !uses_template_parms (CLASSTYPE_TI_ARGS (type));
 }
 
+/* Print the list of overloaded FNS in an error message.   */
+
+static void
+print_overloaded_functions (tree fns, const char **str)
+{
+  tree fn;
+  for (fn = fns; fn; fn = OVL_NEXT (fn))
+    {
+      if (TREE_CODE (fn) == TREE_LIST)
+	print_candidates (fn);
+      else
+	error ("%s %+#D", *str, OVL_CURRENT (fn));
+      *str = "               ";
+    }
+}
+
 /* Print the list of candidate FNS in an error message.  */
 
 void
 print_candidates (tree fns)
 {
-  tree fn;
-  tree f;
-
   const char *str = "candidates are:";
 
   if (is_overloaded_fn (fns))
+    print_overloaded_functions (fns, &str);
+  else
     {
-      for (f = fns; f; f = OVL_NEXT (f))
-	{
-	  error ("%s %+#D", str, OVL_CURRENT (f));
-	  str = "               ";
-	}
+      tree fn;
+      for (fn = fns; fn != NULL_TREE; fn = TREE_CHAIN (fn))
+	print_overloaded_functions (TREE_VALUE (fn), &str);
     }
-  else for (fn = fns; fn != NULL_TREE; fn = TREE_CHAIN (fn))
-    {
-      for (f = TREE_VALUE (fn); f; f = OVL_NEXT (f))
-	error ("%s %+#D", str, OVL_CURRENT (f));
-      str = "               ";
-    }
 }
 
 /* Returns the template (one of the functions given by TEMPLATE_ID)
Index: cp/parser.c
===================================================================
--- cp/parser.c	(revision 154697)
+++ cp/parser.c	(working copy)
@@ -15738,14 +15738,8 @@ cp_parser_class_name (cp_parser *parser,
 					identifier_token->location);
 	  if (ambiguous_decls)
 	    {
-	      error_at (identifier_token->location,
-			"reference to %qD is ambiguous", identifier);
-	      print_candidates (ambiguous_decls);
 	      if (cp_parser_parsing_tentatively (parser))
-		{
-		  identifier_token->ambiguous_p = true;
-		  cp_parser_simulate_error (parser);
-		}
+		cp_parser_simulate_error (parser);
 	      return error_mark_node;
 	    }
 	}

Re: [C++ Patch] Fix PR 35112

[Gabriel Dos Reis]

On Fri, Nov 27, 2009 at 3:29 AM, Paolo Carlini <paolo.carlini@oracle.com> wrote:
> Hi again,
>> Don't pretend I'm that difficult :-)
>>
>> Seriously, here is what I meant:  in the current print_candidates
>> the inner for loop i(the one that goes over functions in an
>> overload set) s repeated twice.  So, you can make
>> that a function of its own, say print_overloaded_functions.
>> Then print_candidates calls print_overloaded_functions in
>> the is_overloaded_fn branch, but loops over the list
>> of overload set in the else branch, calling print_overloaded_functions
>> with the TREE_VALUE.  Does that make sense to you?
>>
> Sure. The below is what I have tested on x86_64-linux. Is it ok?

OK!  Thanks.

Just for my curiosity, was the test for TREE_LIST in print_overloaded_functions
necessary?  For which case?

-- Gaby

Re: [C++ Patch] Fix PR 35112

[Paolo Carlini]

Hi,
> OK! Thanks.
Thank you
> Just for my curiosity, was the test for TREE_LIST in print_overloaded_functions
> necessary?  For which case?
>   
Yes, it was, sorry if my first message wasn't completely explicit to
begin with. The testcase at issue is parse/crash53.C here, which,
without patch causes:

35112.C:13:1: error: reference to ‘A’ is ambiguous
#‘tree_list’ not supported by dump_decl#<declaration error>
tree check: expected tree that contains ‘decl minimal’ structure, have
‘tree_list’ in location_of, at cp/error.c:2410
Please submit a full bug report,
...

Essentially the problem happens with > 2 ambiguous A in different
namespaces.

Paolo.

Re: [C++ Patch] Fix PR 35112

[Gabriel Dos Reis]

On Fri, Nov 27, 2009 at 4:19 AM, Paolo Carlini <paolo.carlini@oracle.com> wrote:
> Hi,
>> OK! Thanks.
> Thank you
>> Just for my curiosity, was the test for TREE_LIST in print_overloaded_functions
>> necessary?  For which case?
>>
> Yes, it was, sorry if my first message wasn't completely explicit to
> begin with. The testcase at issue is parse/crash53.C here, which,
> without patch causes:
>
> 35112.C:13:1: error: reference to ‘A’ is ambiguous
> #‘tree_list’ not supported by dump_decl#<declaration error>
> tree check: expected tree that contains ‘decl minimal’ structure, have
> ‘tree_list’ in location_of, at cp/error.c:2410
> Please submit a full bug report,
> ...
>
> Essentially the problem happens with > 2 ambiguous A in different
> namespaces.

Thanks for this excellent explanation.

-- Gaby