[C++] Prefer error + inform to two errors in check_template_shadow

Hi again,

On 07/14/2015 05:43 PM, Paolo Carlini wrote:
> Hi,
>
> On 07/14/2015 05:07 PM, Jason Merrill wrote:
>> On 07/13/2015 09:41 AM, Paolo Carlini wrote:
>>> +++ testsuite/g++.dg/template/crash81.C (working copy)
>>> @@ -3,6 +3,6 @@
>>>   struct A
>>>   {
>>>     template<T::X> struct X; // { dg-error "'T' has not been 
>>> declared" "T" }
>>> -  // { dg-error "declaration of 'template<int X> struct A::X'" 
>>> "A::X" { target *-*-* } 5 }
>>> -  // { dg-error "shadows template parm 'int X'" "shadow" { target 
>>> *-*-* } 5 }
>>> +  // { dg-error "declaration of 'template<int X> struct A::X' 
>>> shadows" "A::X" { target *-*-* } 5 }
>>> +  // { dg-message "template parameter 'X'" "" { target *-*-* } 5 }
>>
>> I don't see any reason to check for specific diagnostics here; the 
>> latter two messages are poor error-recovery, not something to test for.
> Indeed, I noticed the error recovery issue, which I didn't know, and 
> considered looking into it, when I'm done with some other things. In 
> the meanwhile I'm shortening the expected error/inform.
I had a look and tried various things... Ultimately the issue is due to 
the type_was_error_mark_node becomes integer_type_node trick in 
grokdeclarator (which I don't like that much ;) Thus for this testcase 
the latter simply returns a PARM_DECL of that type and no trace remains 
of the error, then the following bad error recovery. Now, 
cp_parser_template_parameter knows how to handle error_mark_node thus in 
principle we should be able to just return it and be done. The only 
problem with that, testsuite-wise, is the very broken cpp/pr64127.C 
which ends up regressing, since we emit a spurious additional error 
about "default argument for template parameter for class enclosing" from 
check_default_tmpl_args. Note that for this specific testcase 
grokdeclarator gets also a null declarator thus for it isn't the same if 
grokdeclarator returns error_mark_node immediately or just sets type = 
error_mark_node at the beginning (which results in grokdeclarator not 
returning an error_mark_node, instead a PARM_DECL with error_mark_node 
as type).

Thus I have two different proposals: 1- More conservative, just set type 
= error_mark_node instead of integer_type_node when we have a template 
parameter; 2- Return error_mark_node immediately and adjust 
check_default_tmpl_args to cope correctly with pr64127.C.

Thanks!
Paolo.

Index: cp/decl.c
===================================================================
--- cp/decl.c	(revision 226028)
+++ cp/decl.c	(working copy)
@@ -9303,7 +9303,13 @@ grokdeclarator (const cp_declarator *declarator,
 		 && current_namespace == global_namespace);
 
       if (type_was_error_mark_node)
-	/* We've already issued an error, don't complain more.  */;
+	{
+	  /* We've already issued an error, don't complain more.  */
+	  if (template_parm_flag)
+	    /* Just return the error_mark_node, cp_parser_template_parameter
+	       knows how to handle it.  */
+	    return error_mark_node;
+	} 
       else if (in_system_header_at (input_location) || flag_ms_extensions)
 	/* Allow it, sigh.  */;
       else if (! is_main)
Index: cp/pt.c
===================================================================
--- cp/pt.c	(revision 226028)
+++ cp/pt.c	(working copy)
@@ -4704,9 +4704,14 @@ check_default_tmpl_args (tree decl, tree parms, bo
       ntparms = TREE_VEC_LENGTH (inner_parms);
       for (i = 0; i < ntparms; ++i)
         {
-          if (TREE_VEC_ELT (inner_parms, i) == error_mark_node)
+	  tree parm = TREE_VEC_ELT (inner_parms, i);
+
+          if (parm == error_mark_node)
             continue;
 
+	  if (TREE_PURPOSE (parm) == error_mark_node)
+	    continue;	  
+
 	  if (TREE_PURPOSE (TREE_VEC_ELT (inner_parms, i)))
 	    {
 	      if (msg)
Index: testsuite/g++.dg/template/crash81.C
===================================================================
--- testsuite/g++.dg/template/crash81.C	(revision 226028)
+++ testsuite/g++.dg/template/crash81.C	(working copy)
@@ -2,6 +2,5 @@
 
 struct A
 {
-  template<T::X> struct X; // { dg-error "'T' has not been declared" "T" }
-  // { dg-bogus "declaration" "" { xfail *-*-* } 5 }
+  template<T::X> struct X; // { dg-error "'T' has not been declared" }
 };

On 07/21/2015 09:02 AM, Paolo Carlini wrote:
> I had a look and tried various things... Ultimately the issue is due to
> the type_was_error_mark_node becomes integer_type_node trick in
> grokdeclarator (which I don't like that much ;)

What happens if we just remove that trick?

Jason

Hi,

On 07/21/2015 06:07 PM, Jason Merrill wrote:
> On 07/21/2015 09:02 AM, Paolo Carlini wrote:
>> I had a look and tried various things... Ultimately the issue is due to
>> the type_was_error_mark_node becomes integer_type_node trick in
>> grokdeclarator (which I don't like that much ;)
> What happens if we just remove that trick?
Removing it completely (thus essentially unconditionally setting back 
type = error_mark_node when type_was_error_mark_node is true) leads to a 
few regressions. I tried that earlier today. For cases like 
other/nontype-1.C for example we do not handle the second error like the 
first one. Thus:

nontype-1.C:3:16: error: ‘Op::first_argument_type’ is not a type
nontype-1.C:4:16: error: ‘Op::second_argument_type’ is not a type

becomes:

nontype-1.C:3:16: error: ‘Op::first_argument_type’ is not a type
nontype-1.C:3:37: error: expected ‘)’ before ‘,’ token
nontype-1.C:3:37: error: expected ‘;’ before ‘,’ token

In other case we get additional "no match for call"-type errors. In the 
case of cpp1y/pr65340.C we loose the error message about "use of 'auto' 
... before deduction of 'auto'". All in all I think there are a few 
tricky cases:

FAIL: g++.dg/cpp0x/alias-decl-33.C  -std=c++11 (test for excess errors)
FAIL: g++.dg/cpp0x/alias-decl-33.C  -std=c++14 (test for excess errors)
FAIL: g++.dg/cpp1y/pr65340.C  -std=c++14  (test for errors, line 15)
FAIL: g++.dg/cpp1y/pr65340.C  -std=c++14 (test for excess errors)
FAIL: g++.dg/inherit/error4.C  -std=c++98  (test for errors, line 7)
FAIL: g++.dg/inherit/error4.C  -std=c++98 (test for excess errors)
FAIL: g++.dg/inherit/error4.C  -std=c++11  (test for errors, line 7)
FAIL: g++.dg/inherit/error4.C  -std=c++11 (test for excess errors)
FAIL: g++.dg/inherit/error4.C  -std=c++14  (test for errors, line 7)
FAIL: g++.dg/inherit/error4.C  -std=c++14 (test for excess errors)
FAIL: g++.dg/other/nontype-1.C  -std=c++98  (test for errors, line 4)
FAIL: g++.dg/other/nontype-1.C  -std=c++98 (test for excess errors)
FAIL: g++.dg/other/nontype-1.C  -std=c++11  (test for errors, line 4)
FAIL: g++.dg/other/nontype-1.C  -std=c++11 (test for excess errors)
FAIL: g++.dg/other/nontype-1.C  -std=c++14  (test for errors, line 4)
FAIL: g++.dg/other/nontype-1.C  -std=c++14 (test for excess errors)
FAIL: g++.dg/parse/enum3.C  -std=c++98  (test for errors, line 3)
FAIL: g++.dg/parse/enum3.C  -std=c++98 (test for excess errors)
FAIL: g++.dg/parse/enum3.C  -std=c++11  (test for errors, line 3)
FAIL: g++.dg/parse/enum3.C  -std=c++11 (test for excess errors)
FAIL: g++.dg/parse/enum3.C  -std=c++14  (test for errors, line 3)
FAIL: g++.dg/parse/enum3.C  -std=c++14 (test for excess errors)
FAIL: g++.dg/parse/error3.C  -std=gnu++98 (test for excess errors)
FAIL: g++.dg/parse/error3.C  -std=gnu++11 (test for excess errors)
FAIL: g++.dg/parse/error3.C  -std=gnu++14 (test for excess errors)
FAIL: g++.dg/template/nontype25.C  -std=c++98 (test for excess errors)
FAIL: g++.dg/template/nontype25.C  -std=c++11  (test for errors, line 10)
FAIL: g++.dg/template/nontype25.C  -std=c++14  (test for errors, line 10)

What about applying something like my first patch with a comment that in 
principle we would like to extend it carefully to more cases outside 
template parameters?

Thanks,
Paolo.

On 07/21/2015 09:58 AM, Paolo Carlini wrote:
> What about applying something like my first patch with a comment that in
> principle we would like to extend it carefully to more cases outside
> template parameters?

OK, sure.

Jason