[C++] Improve cp_parser_class_head error recovery
diff mbox series

Message ID eb8db7d6-757d-cbd0-87b0-2410310ec930@oracle.com
State New
Headers show
Series
  • [C++] Improve cp_parser_class_head error recovery
Related show

Commit Message

Paolo Carlini Oct. 18, 2019, 3:35 p.m. UTC
Hi,

a few days ago I noticed that for, say, g++.dg/parse/qualified2.C we 
were issuing two additional misleading errors after the first one, 
mentioning in particular a certain "unnamed class" (I'm reproducing only 
the error messages proper):

namespace Glib {
   template <typename> class Value {};
   template <>         class Glib::Value<int> {}; // { dg-error "" }
}

qualified2.C:3:29: error: extra qualification not allowed [\-fpermissive\]
qualified2.C:3:46: error: explicit specialization of non-template 
‘Glib::<unnamed class>’
qualified2.C:3:47: error: abstract declarator ‘Glib::<unnamed class>’ 
used as declaration

Let's see if I can explain clearly enough what I think it's going on.

In cp_parser_class_head, upon the permerror about the extra 
qualification, we try to do error recovery, which is particularly 
tricky, because we are dealing with a permerror thus we have to make 
sure that in case of -fpermissive everything remains internally 
consistent anyway. In this context, clearing 'nested_name_specifier' and 
'num_templates' doesn't seem a good idea because it does *not* give us 
an internal state similar to the one normally obtained when the nested 
name specifier is not there, the reason being that, earlier in the 
function, when a nested name specifier really isn't there we try 
cp_parser_template_id or in case cp_parser_identifier, which set the 
locale 'id' and possibly 'template_id' and 'num_templates', whereas 
during error recovery we remain so to speak completely empty handed. 
Thus, what about not clearing anything? That seems to work at least for 
the two testcases below and doesn't cause regressions.

Thanks, Paolo.

/////////////////////////////
/cp
2019-10-18  Paolo Carlini  <paolo.carlini@oracle.com>

	* parser.c (cp_parser_class_head): Improve error recovery upon
	extra qualification error.

/testsuite
2019-10-18  Paolo Carlini  <paolo.carlini@oracle.com>

	* g++.dg/parse/qualified2.C: Tighten dg-error directive.
	* g++.old-deja/g++.other/decl5.C: Don't expect redundant error.

Comments

Jason Merrill Oct. 21, 2019, 6:23 p.m. UTC | #1
On 10/18/19 11:35 AM, Paolo Carlini wrote:
> Hi,
> 
> a few days ago I noticed that for, say, g++.dg/parse/qualified2.C we 
> were issuing two additional misleading errors after the first one, 
> mentioning in particular a certain "unnamed class" (I'm reproducing only 
> the error messages proper):
> 
> namespace Glib {
>    template <typename> class Value {};
>    template <>         class Glib::Value<int> {}; // { dg-error "" }
> }
> 
> qualified2.C:3:29: error: extra qualification not allowed [\-fpermissive\]
> qualified2.C:3:46: error: explicit specialization of non-template 
> ‘Glib::<unnamed class>’
> qualified2.C:3:47: error: abstract declarator ‘Glib::<unnamed class>’ 
> used as declaration
> 
> Let's see if I can explain clearly enough what I think it's going on.
> 
> In cp_parser_class_head, upon the permerror about the extra 
> qualification, we try to do error recovery, which is particularly 
> tricky, because we are dealing with a permerror thus we have to make 
> sure that in case of -fpermissive everything remains internally 
> consistent anyway. In this context, clearing 'nested_name_specifier' and 
> 'num_templates' doesn't seem a good idea because it does *not* give us 
> an internal state similar to the one normally obtained when the nested 
> name specifier is not there, the reason being that, earlier in the 
> function, when a nested name specifier really isn't there we try 
> cp_parser_template_id or in case cp_parser_identifier, which set the 
> locale 'id' and possibly 'template_id' and 'num_templates', whereas 
> during error recovery we remain so to speak completely empty handed. 
> Thus, what about not clearing anything? That seems to work at least for 
> the two testcases below and doesn't cause regressions.
> 
> Thanks, Paolo.
> 
> /////////////////////////////
> 
> 
> 
OK.

Jason

Patch
diff mbox series

Index: cp/parser.c
===================================================================
--- cp/parser.c	(revision 277149)
+++ cp/parser.c	(working copy)
@@ -24178,12 +24178,8 @@  cp_parser_class_head (cp_parser* parser,
 	 ... [or] the definition or explicit instantiation of a
 	 class member of a namespace outside of its namespace.  */
       if (scope == nested_name_specifier)
-	{
-	  permerror (nested_name_specifier_token_start->location,
-		     "extra qualification not allowed");
-	  nested_name_specifier = NULL_TREE;
-	  num_templates = 0;
-	}
+	permerror (nested_name_specifier_token_start->location,
+		   "extra qualification not allowed");
     }
   /* An explicit-specialization must be preceded by "template <>".  If
      it is not, try to recover gracefully.  */
Index: testsuite/g++.dg/parse/qualified2.C
===================================================================
--- testsuite/g++.dg/parse/qualified2.C	(revision 277144)
+++ testsuite/g++.dg/parse/qualified2.C	(working copy)
@@ -1,4 +1,4 @@ 
 namespace Glib {
   template <typename> class Value {};
-  template <>         class Glib::Value<int> {}; // { dg-error "" }
+  template <>         class Glib::Value<int> {}; // { dg-error "29:extra qualification" }
 }
Index: testsuite/g++.old-deja/g++.other/decl5.C
===================================================================
--- testsuite/g++.old-deja/g++.other/decl5.C	(revision 277144)
+++ testsuite/g++.old-deja/g++.other/decl5.C	(working copy)
@@ -12,7 +12,6 @@  struct A {
   int A::m;           // { dg-error "extra qualification" } 
   struct e;
   struct A::e {int i;}; // { dg-error "extra qualification" "qual" } 
-  // { dg-error "anonymous struct" "anon" { target *-*-* } .-1 }
   struct A::expand {  // { dg-error "qualified name" } 
   int m;
   };