diff mbox

[c++] fix PR 4452{2,3}, autocorrect colon to scopes when appropriate

Message ID 20101208204632.GD25904@nightcrawler
State New
Headers show

Commit Message

Nathan Froyd Dec. 8, 2010, 8:46 p.m. UTC 
The patch below addresses PRs 44522 and 44523, which both request that a
colon in a nested-name-specifier be auto-corrected to a scope (::).  The
implementation below adds a flag to cp_parser indicating whether it's
valid to do that.  The flag defaults to "yes, it's valid" and gets
turned off when parsing certain productions (e.g. parsing case labels,
or parsing the start of class declarations).

The idea is pretty simple; most of the hair in the patch below involves
making sure that functions where we temporarily set
colon_corrects_to_scope_p exit through a single point, where we can then
reset colon_corrects_to_scope_p.  I'm happy to leave the code the way it
is and duplicate the resetting at appropriate return points if the
maintainers think that would be a better idea.

Ideas on improvements would be welcome; I went for the approach of
making sure the testcases work (and trying out a few other cases) and
making sure the testsuites pass.  My C++ is pretty weak, so there's
probably interesting cases that I don't catch.

Tested on x86_64-unknown-linux-gnu (g++ and libstdc++ testsuites).  OK
to commit?

-Nathan

gcc/cp/
	PR c++/44522
	PR c++/44523
	* parser.c (struct cp_parser): Add colon_corrects_to_scope_p field.
	(cp_parser_new): Initialize it.
	(cp_parser_nested_name_specifier_opt): Auto-correct colons to
	scopes if we are able to.
	(cp_parser_question_colon_clause): Disallow colon correction.
	(cp_parser_label_for_labeled_statement): Likewise.
	(cp_parser_range_for): Likewise.
	(cp_parser_enum_specifier): Likewise.
	(cp_parser_class_head): Likewise.
	(cp_parser_member_declaration): Likewise.

gcc/testsuite/
	PR c++/44522
	PR c++/44523
	* g++.dg/parse/colon-autocorrect-1.C: New testcase.
	* g++.dg/parse/colon-autocorrect-2.C: New testcase.

Comments

Dodji Seketeli Dec. 9, 2010, 3:13 p.m. UTC | #1 
Hello Nathan,

FWIW, I find this pretty cool. Thank you for working on this.

I just have a small question.

Nathan Froyd <froydnj@codesourcery.com> writes:

[...]

> @@ -4553,6 +4559,16 @@ cp_parser_nested_name_specifier_opt (cp_parser *parser,
>  	     template-id), nor a `::', then we are not looking at a
>  	     nested-name-specifier.  */
>  	  token = cp_lexer_peek_nth_token (parser->lexer, 2);
> +
> +	  if (token->type == CPP_COLON
> +	      && parser->colon_corrects_to_scope_p
> +	      && cp_lexer_peek_nth_token (parser->lexer, 3)->type == CPP_NAME)
> +	    {
> +	      error_at (token->location,
> +			"found %<:%> in nested-name-specifier, expected %<::%>");
> +	      token->type = CPP_SCOPE;
> +	    }
> +

I wonder how this would behave if cp_parser_nested_name_specifier_opt is
called during a tentative parse that should (silently) fail. In that
case my understanding is we would issue an error message even though we
are just parsing tentatively.

Maybe we could disable the auto-correcting in the if at the begining of
cp_parser_nested_name_specifier_opt:

  /* Remember where the nested-name-specifier starts.  */
  if (cp_parser_uncommitted_to_tentative_parse_p (parser))
    {
      start = cp_lexer_token_position (parser->lexer, false);
      push_deferring_access_checks (dk_deferred);
    }

Also, I am thinking maybe enabling the auto-correcting by default and
picking where not to enable it can let us allow ':' instead of :: in too
many places. Perhaps we should rather disable auto-correcting by default
and just enable it during the parsing of a simple-declaration. Maybe I
am just being overly conservative :-)
Nathan Froyd Dec. 9, 2010, 3:55 p.m. UTC | #2 
On Thu, Dec 09, 2010 at 04:13:06PM +0100, Dodji Seketeli wrote:
> Nathan Froyd <froydnj@codesourcery.com> writes:
> > @@ -4553,6 +4559,16 @@ cp_parser_nested_name_specifier_opt (cp_parser *parser,
> >  	     template-id), nor a `::', then we are not looking at a
> >  	     nested-name-specifier.  */
> >  	  token = cp_lexer_peek_nth_token (parser->lexer, 2);
> > +
> > +	  if (token->type == CPP_COLON
> > +	      && parser->colon_corrects_to_scope_p
> > +	      && cp_lexer_peek_nth_token (parser->lexer, 3)->type == CPP_NAME)
> > +	    {
> > +	      error_at (token->location,
> > +			"found %<:%> in nested-name-specifier, expected %<::%>");
> > +	      token->type = CPP_SCOPE;
> > +	    }
> > +
> 
> I wonder how this would behave if cp_parser_nested_name_specifier_opt is
> called during a tentative parse that should (silently) fail. In that
> case my understanding is we would issue an error message even though we
> are just parsing tentatively.

That's a good question.  I know that this code gets hit when parsing
tenatively (see the testcase in PR 39859, probably also the testcases I
included with the patch).  I don't know whether there's a case where we
could auto-correct, issue an error, fail, reparse with no
auto-correction, and succeed.  Hopefully there's no cases where that
would happen; I don't think the auto-correct would be very useful if we
limited it to non-tenative parses.

> Also, I am thinking maybe enabling the auto-correcting by default and
> picking where not to enable it can let us allow ':' instead of :: in too
> many places. Perhaps we should rather disable auto-correcting by default
> and just enable it during the parsing of a simple-declaration. Maybe I
> am just being overly conservative :-)

I think the optimistic view (auto-correct always, turn it off when you
can't) is somewhat more feasible; there's only so many places in the
grammar where you can have `:' and only so many places where you'd want
to correct it.

-Nathan
Jason Merrill Dec. 15, 2010, 4:46 p.m. UTC | #3 
On 12/09/2010 10:55 AM, Nathan Froyd wrote:
> That's a good question.  I know that this code gets hit when parsing
> tenatively (see the testcase in PR 39859, probably also the testcases I
> included with the patch).  I don't know whether there's a case where we
> could auto-correct, issue an error, fail, reparse with no
> auto-correction, and succeed.  Hopefully there's no cases where that
> would happen; I don't think the auto-correct would be very useful if we
> limited it to non-tenative parses.

Since the patch specifically disables auto-correction in situations 
where a colon could be correct, I don't think this is a danger.

The patch is OK.

Jason
diff mbox

Patch

diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
index ab533f4..b31bf01 100644
--- a/gcc/cp/parser.c
+++ b/gcc/cp/parser.c
@@ -1700,6 +1700,9 @@  typedef struct GTY(()) cp_parser {
      a local class.  */
   bool in_function_body;
 
+  /* TRUE if we can auto-correct a colon to a scope operator.  */
+  bool colon_corrects_to_scope_p;
+
   /* If non-NULL, then we are parsing a construct where new type
      definitions are not permitted.  The string stored here will be
      issued as an error message if a type is defined.  */
@@ -3244,6 +3247,9 @@  cp_parser_new (void)
   /* We are not parsing a function body.  */
   parser->in_function_body = false;
 
+  /* We can correct until told otherwise.  */
+  parser->colon_corrects_to_scope_p = true;
+
   /* The unparsed function queue is empty.  */
   push_unparsed_function_queues (parser);
 
@@ -4553,6 +4559,16 @@  cp_parser_nested_name_specifier_opt (cp_parser *parser,
 	     template-id), nor a `::', then we are not looking at a
 	     nested-name-specifier.  */
 	  token = cp_lexer_peek_nth_token (parser->lexer, 2);
+
+	  if (token->type == CPP_COLON
+	      && parser->colon_corrects_to_scope_p
+	      && cp_lexer_peek_nth_token (parser->lexer, 3)->type == CPP_NAME)
+	    {
+	      error_at (token->location,
+			"found %<:%> in nested-name-specifier, expected %<::%>");
+	      token->type = CPP_SCOPE;
+	    }
+
 	  if (token->type != CPP_SCOPE
 	      && !cp_parser_nth_token_starts_template_argument_list_p
 		  (parser, 2))
@@ -6955,12 +6971,15 @@  cp_parser_question_colon_clause (cp_parser* parser, tree logical_or_expr)
     }
   else
     {
+      bool saved_colon_corrects_to_scope_p = parser->colon_corrects_to_scope_p;
+      parser->colon_corrects_to_scope_p = false;
       /* Parse the expression.  */
       c_inhibit_evaluation_warnings += logical_or_expr == truthvalue_false_node;
       expr = cp_parser_expression (parser, /*cast_p=*/false, NULL);
       c_inhibit_evaluation_warnings +=
 	((logical_or_expr == truthvalue_true_node)
 	 - (logical_or_expr == truthvalue_false_node));
+      parser->colon_corrects_to_scope_p = saved_colon_corrects_to_scope_p;
     }
 
   /* The next token should be a `:'.  */
@@ -8153,6 +8172,7 @@  cp_parser_label_for_labeled_statement (cp_parser* parser)
 {
   cp_token *token;
   tree label = NULL_TREE;
+  bool saved_colon_corrects_to_scope_p = parser->colon_corrects_to_scope_p;
 
   /* The next token should be an identifier.  */
   token = cp_lexer_peek_token (parser->lexer);
@@ -8163,6 +8183,7 @@  cp_parser_label_for_labeled_statement (cp_parser* parser)
       return;
     }
 
+  parser->colon_corrects_to_scope_p = false;
   switch (token->keyword)
     {
     case RID_CASE:
@@ -8241,6 +8262,8 @@  cp_parser_label_for_labeled_statement (cp_parser* parser)
       else
 	cplus_decl_attributes (&label, attrs, 0);
     }
+
+  parser->colon_corrects_to_scope_p = saved_colon_corrects_to_scope_p;
 }
 
 /* Parse an expression-statement.
@@ -8710,7 +8733,9 @@  cp_parser_range_for (cp_parser *parser)
   cp_declarator *declarator;
   const char *saved_message;
   tree attributes, pushed_scope;
+  bool saved_colon_corrects_to_scope_p = parser->colon_corrects_to_scope_p;
 
+  parser->colon_corrects_to_scope_p = false;
   cp_parser_parse_tentatively (parser);
   /* New types are not allowed in the type-specifier-seq for a
      range-based for loop.  */
@@ -8727,7 +8752,8 @@  cp_parser_range_for (cp_parser *parser)
   if (cp_parser_error_occurred (parser))
     {
       cp_parser_abort_tentative_parse (parser);
-      return NULL_TREE;
+      stmt = NULL_TREE;
+      goto out;
     }
   /* Parse the declarator.  */
   declarator = cp_parser_declarator (parser, CP_PARSER_DECLARATOR_NAMED,
@@ -8774,6 +8800,8 @@  cp_parser_range_for (cp_parser *parser)
     /* Convert the range-based for loop into a normal for-statement. */
     stmt = cp_convert_range_for (stmt, range_decl, range_expr);
 
+ out:
+  parser->colon_corrects_to_scope_p = saved_colon_corrects_to_scope_p;
   return stmt;
 }
 
@@ -13343,6 +13371,9 @@  cp_parser_enum_specifier (cp_parser* parser)
   bool is_anonymous = false;
   tree underlying_type = NULL_TREE;
   cp_token *type_start_token = NULL;
+  bool saved_colon_corrects_to_scope_p = parser->colon_corrects_to_scope_p;
+
+  parser->colon_corrects_to_scope_p = false;
 
   /* Parse tentatively so that we can back up if we don't find a
      enum-specifier.  */
@@ -13470,7 +13501,10 @@  cp_parser_enum_specifier (cp_parser* parser)
 	{
 	  cp_parser_error (parser, "expected %<{%>");
 	  if (has_underlying_type)
-	    return NULL_TREE;
+	    {
+	      type = NULL_TREE;
+	      goto out;
+	    }
 	}
       /* An opaque-enum-specifier must have a ';' here.  */
       if ((scoped_enum_p || underlying_type)
@@ -13478,7 +13512,10 @@  cp_parser_enum_specifier (cp_parser* parser)
 	{
 	  cp_parser_error (parser, "expected %<;%> or %<{%>");
 	  if (has_underlying_type)
-	    return NULL_TREE;
+	    {
+	      type = NULL_TREE;
+	      goto out;
+	    }
 	}
     }
 
@@ -13622,6 +13659,8 @@  cp_parser_enum_specifier (cp_parser* parser)
 	  pop_nested_namespace (nested_name_specifier);
 	}
     }
+ out:
+  parser->colon_corrects_to_scope_p = saved_colon_corrects_to_scope_p;
   return type;
 }
 
@@ -17085,6 +17124,7 @@  cp_parser_class_head (cp_parser* parser,
   bool qualified_p = false;
   bool invalid_nested_name_p = false;
   bool invalid_explicit_specialization_p = false;
+  bool saved_colon_corrects_to_scope_p = parser->colon_corrects_to_scope_p;
   tree pushed_scope = NULL_TREE;
   unsigned num_templates;
   cp_token *type_start_token = NULL, *nested_name_specifier_token_start = NULL;
@@ -17093,6 +17133,7 @@  cp_parser_class_head (cp_parser* parser,
   /* Assume no template parameter lists will be used in defining the
      type.  */
   num_templates = 0;
+  parser->colon_corrects_to_scope_p = false;
 
   *bases = NULL_TREE;
 
@@ -17232,7 +17273,8 @@  cp_parser_class_head (cp_parser* parser,
   if (!cp_parser_next_token_starts_class_definition_p (parser))
     {
       cp_parser_error (parser, "expected %<{%> or %<:%>");
-      return error_mark_node;
+      type = error_mark_node;
+      goto out;
     }
 
   /* At this point, we're going ahead with the class-specifier, even
@@ -17243,13 +17285,15 @@  cp_parser_class_head (cp_parser* parser,
     {
       cp_parser_error (parser,
 		       "global qualification of class name is invalid");
-      return error_mark_node;
+      type = error_mark_node;
+      goto out;
     }
   else if (invalid_nested_name_p)
     {
       cp_parser_error (parser,
 		       "qualified name does not name a class");
-      return error_mark_node;
+      type = error_mark_node;
+      goto out;
     }
   else if (nested_name_specifier)
     {
@@ -17452,6 +17496,8 @@  cp_parser_class_head (cp_parser* parser,
   if (type)
     DECL_SOURCE_LOCATION (TYPE_NAME (type)) = type_start_token->location;
   *attributes_p = attributes;
+ out:
+  parser->colon_corrects_to_scope_p = saved_colon_corrects_to_scope_p;
   return type;
 }
 
@@ -17580,6 +17626,7 @@  cp_parser_member_declaration (cp_parser* parser)
   cp_token *decl_spec_token_start = NULL;
   cp_token *initializer_token_start = NULL;
   int saved_pedantic;
+  bool saved_colon_corrects_to_scope_p = parser->colon_corrects_to_scope_p;
 
   /* Check for the `__extension__' keyword.  */
   if (cp_parser_extension_opt (parser, &saved_pedantic))
@@ -17638,8 +17685,10 @@  cp_parser_member_declaration (cp_parser* parser)
       return;
     }
 
+  parser->colon_corrects_to_scope_p = false;
+
   if (cp_parser_using_declaration (parser, /*access_declaration=*/true))
-    return;
+    goto out;
 
   /* Parse the decl-specifier-seq.  */
   decl_spec_token_start = cp_lexer_peek_token (parser->lexer);
@@ -17652,7 +17701,7 @@  cp_parser_member_declaration (cp_parser* parser)
   /* Check for an invalid type-name.  */
   if (!decl_specifiers.any_type_specifiers_p
       && cp_parser_parse_and_diagnose_invalid_type_name (parser))
-    return;
+    goto out;
   /* If there is no declarator, then the decl-specifier-seq should
      specify a type.  */
   if (cp_lexer_next_token_is (parser->lexer, CPP_SEMICOLON))
@@ -17822,7 +17871,7 @@  cp_parser_member_declaration (cp_parser* parser)
 		  if (cp_lexer_next_token_is (parser->lexer,
 					      CPP_SEMICOLON))
 		    cp_lexer_consume_token (parser->lexer);
-		  return;
+		  goto out;
 		}
 
 	      if (declares_class_or_enum & 2)
@@ -17901,7 +17950,7 @@  cp_parser_member_declaration (cp_parser* parser)
 		  /* If the next token is a semicolon, consume it.  */
 		  if (token->type == CPP_SEMICOLON)
 		    cp_lexer_consume_token (parser->lexer);
-		  return;
+		  goto out;
 		}
 	      else
 		if (declarator->kind == cdk_function)
@@ -17956,11 +18005,13 @@  cp_parser_member_declaration (cp_parser* parser)
 	    }
 
 	  if (assume_semicolon)
-	    return;
+	    goto out;
 	}
     }
 
   cp_parser_require (parser, CPP_SEMICOLON, RT_SEMICOLON);
+ out:
+  parser->colon_corrects_to_scope_p = saved_colon_corrects_to_scope_p;
 }
 
 /* Parse a pure-specifier.
diff --git a/gcc/testsuite/g++.dg/parse/colon-autocorrect-1.C b/gcc/testsuite/g++.dg/parse/colon-autocorrect-1.C
new file mode 100644
index 0000000..8e25fba
--- /dev/null
+++ b/gcc/testsuite/g++.dg/parse/colon-autocorrect-1.C
@@ -0,0 +1,31 @@ 
+// PR c++/44522
+// { dg-do compile }
+
+namespace x {
+  struct a { };
+  a A0;
+}
+
+x:a a2;				// { dg-error "nested-name-specifier" }
+x::a a3 = a2;
+
+x:a f (void)			// { dg-error "nested-name-specifier" }
+{
+  x::a a4;			// x:a would parse like a label
+  return a4;
+}
+
+x::a g (x:a a4)			// { dg-error "nested-name-specifier" }
+{
+  return a4;
+}
+
+class B
+{
+  x::a f(void)			// x:a would parse like a bitfield
+  {
+    x::a a4;
+    a4 = x:A0;			// { dg-error "nested-name-specifier" }
+    return a4;
+  }
+};
diff --git a/gcc/testsuite/g++.dg/parse/colon-autocorrect-2.C b/gcc/testsuite/g++.dg/parse/colon-autocorrect-2.C
new file mode 100644
index 0000000..1dfcbc0
--- /dev/null
+++ b/gcc/testsuite/g++.dg/parse/colon-autocorrect-2.C
@@ -0,0 +1,15 @@ 
+// PR c++/44523
+// { dg-do compile }
+
+namespace x {
+  struct a { };
+}
+
+template <typename t>
+class foo {
+};
+
+foo<x::a> a1;
+foo<x:a> a2;			// { dg-error "nested-name-specifier" }
+
+x::a a3 = a2;			// { dg-error "conversion" }