Patchwork [PING] PR c++/54401 - Confusing diagnostics about type-alias at class scope

login
register
mail settings
Submitter Dodji Seketeli
Date Dec. 7, 2012, 1:33 p.m.
Message ID <87zk1qasvv.fsf@redhat.com>
Download mbox | patch
Permalink /patch/204488/
State New
Headers show

Comments

Dodji Seketeli - Dec. 7, 2012, 1:33 p.m.
Jason Merrill <jason@redhat.com> writes:

> Oops, thought I had sent this before.

No problem.  Thank you for replying now×

> On 11/17/2012 10:23 AM, Dodji Seketeli wrote:
> > -	  if (cp_parser_parse_definitely (parser))
> > +	  /* Note that if we actually see the '=' token after the
> > +	     identifier, cp_parser_alias_declaration commits the
> > +	     tentative parse.  In that case, we really expects an
> > +	     alias-declaration.  Otherwise, we expect a using
> > +	     declaration.  */
> > +	  alias_decl_expected =
> > +	    !cp_parser_uncommitted_to_tentative_parse_p (parser);
> > +	  cp_parser_parse_definitely (parser);
> 
> Maybe just check whether decl is error_mark_node?

As we further discussed this on IRC, when cp_parser_alias_declaration
returns error_mark_node, it is either because we encountered an error
before the hypothetical '=', in which case we might want to try to
parse a using declaration, or it is because we encountered an error
after the the '=', in which case we committed to parsing an alias
declaration and we must emit a hard error.

And I believe what we want here is to tell the two possible cases
apart.

> 
> > +  if (type == error_mark_node)
> > +    return error_mark_node;
> 
> I think for error-recovery we might want to
> cp_parser_skip_to_end_of_block_or_statement as well.

Right, done.

Bootstrapped and tested on x86_64-unknown-linux-gnu against trunk.

gcc/cp/

	* parser.c (cp_parser_alias_declaration): Commit to tentative
	parse when see the '=' token.  Get out if the type-id is invalid.
	Update function comment.
	(cp_parser_member_declaration): Don't try to parse a using
	declaration if we know that we expected an alias declaration; that
	is, if we see the '=' token after the identifier.

gcc/testsuite/

	* g++.dg/cpp0x/alias-decl-28.C: New test.
	* g++.dg/cpp0x/alias-decl-16.C: Update.
---
 gcc/cp/parser.c                            | 31 +++++++++++++++++++++++++++---
 gcc/testsuite/g++.dg/cpp0x/alias-decl-16.C |  2 +-
 gcc/testsuite/g++.dg/cpp0x/alias-decl-28.C |  7 +++++++
 3 files changed, 36 insertions(+), 4 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/cpp0x/alias-decl-28.C
Gabriel Dos Reis - Dec. 7, 2012, 1:42 p.m.
On Fri, Dec 7, 2012 at 7:33 AM, Dodji Seketeli <dodji@redhat.com> wrote:
> Jason Merrill <jason@redhat.com> writes:
>
>> Oops, thought I had sent this before.
>
> No problem.  Thank you for replying now×
>
>> On 11/17/2012 10:23 AM, Dodji Seketeli wrote:
>> > -     if (cp_parser_parse_definitely (parser))
>> > +     /* Note that if we actually see the '=' token after the
>> > +        identifier, cp_parser_alias_declaration commits the
>> > +        tentative parse.  In that case, we really expects an
>> > +        alias-declaration.  Otherwise, we expect a using
>> > +        declaration.  */
>> > +     alias_decl_expected =
>> > +       !cp_parser_uncommitted_to_tentative_parse_p (parser);
>> > +     cp_parser_parse_definitely (parser);
>>
>> Maybe just check whether decl is error_mark_node?
>
> As we further discussed this on IRC, when cp_parser_alias_declaration
> returns error_mark_node, it is either because we encountered an error
> before the hypothetical '=', in which case we might want to try to
> parse a using declaration, or it is because we encountered an error
> after the the '=', in which case we committed to parsing an alias
> declaration and we must emit a hard error.
>
> And I believe what we want here is to tell the two possible cases
> apart.

Yes!

>
>>
>> > +  if (type == error_mark_node)
>> > +    return error_mark_node;
>>
>> I think for error-recovery we might want to
>> cp_parser_skip_to_end_of_block_or_statement as well.
>
> Right, done.
>
> Bootstrapped and tested on x86_64-unknown-linux-gnu against trunk.
>
> gcc/cp/
>
>         * parser.c (cp_parser_alias_declaration): Commit to tentative
>         parse when see the '=' token.  Get out if the type-id is invalid.
>         Update function comment.
>         (cp_parser_member_declaration): Don't try to parse a using
>         declaration if we know that we expected an alias declaration; that
>         is, if we see the '=' token after the identifier.
>
> gcc/testsuite/
>
>         * g++.dg/cpp0x/alias-decl-28.C: New test.
>         * g++.dg/cpp0x/alias-decl-16.C: Update.
> ---
>  gcc/cp/parser.c                            | 31 +++++++++++++++++++++++++++---
>  gcc/testsuite/g++.dg/cpp0x/alias-decl-16.C |  2 +-
>  gcc/testsuite/g++.dg/cpp0x/alias-decl-28.C |  7 +++++++
>  3 files changed, 36 insertions(+), 4 deletions(-)
>  create mode 100644 gcc/testsuite/g++.dg/cpp0x/alias-decl-28.C
>
> diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
> index a010f1f..c39ef30 100644
> --- a/gcc/cp/parser.c
> +++ b/gcc/cp/parser.c
> @@ -15262,7 +15262,11 @@ cp_parser_using_declaration (cp_parser* parser,
>  /* Parse an alias-declaration.
>
>     alias-declaration:
> -     using identifier attribute-specifier-seq [opt] = type-id  */
> +     using identifier attribute-specifier-seq [opt] = type-id
> +
> +   Note that if this function is used in the context of a tentative
> +   parse, it commits the currently active tentative parse after it
> +   sees the '=' token.  */

This comment is repeated below; I don't think it is necessary
or should be part of the specification.  In general we don't (and shouldn't)
say this unless a parsing function deviates from the general
parsing strategy -- this one doesn't.


>
>  static tree
>  cp_parser_alias_declaration (cp_parser* parser)
> @@ -15295,6 +15299,8 @@ cp_parser_alias_declaration (cp_parser* parser)
>    if (cp_parser_error_occurred (parser))
>      return error_mark_node;
>
> +  cp_parser_commit_to_tentative_parse (parser);
> +
>    /* Now we are going to parse the type-id of the declaration.  */
>
>    /*
> @@ -15324,10 +15330,19 @@ cp_parser_alias_declaration (cp_parser* parser)
>    if (parser->num_template_parameter_lists)
>      parser->type_definition_forbidden_message = saved_message;
>
> +  if (type == error_mark_node)
> +    {
> +      cp_parser_skip_to_end_of_block_or_statement (parser);
> +      return error_mark_node;
> +    }
> +
>    cp_parser_require (parser, CPP_SEMICOLON, RT_SEMICOLON);
>
>    if (cp_parser_error_occurred (parser))
> -    return error_mark_node;
> +    {
> +      cp_parser_skip_to_end_of_block_or_statement (parser);
> +      return error_mark_node;
> +    }
>
>    /* A typedef-name can also be introduced by an alias-declaration. The
>       identifier following the using keyword becomes a typedef-name. It has
> @@ -19063,9 +19078,19 @@ cp_parser_member_declaration (cp_parser* parser)
>        else
>         {
>           tree decl;
> +         bool alias_decl_expected;
>           cp_parser_parse_tentatively (parser);
>           decl = cp_parser_alias_declaration (parser);
> -         if (cp_parser_parse_definitely (parser))
> +         /* Note that if we actually see the '=' token after the
> +            identifier, cp_parser_alias_declaration commits the
> +            tentative parse.  In that case, we really expects an
> +            alias-declaration.  Otherwise, we expect a using
> +            declaration.  */
> +         alias_decl_expected =
> +           !cp_parser_uncommitted_to_tentative_parse_p (parser);
> +         cp_parser_parse_definitely (parser);
> +
> +         if (alias_decl_expected)
>             finish_member_declaration (decl);
>           else
>             cp_parser_using_declaration (parser,
> diff --git a/gcc/testsuite/g++.dg/cpp0x/alias-decl-16.C b/gcc/testsuite/g++.dg/cpp0x/alias-decl-16.C
> index d66660a..ce6ad0a 100644
> --- a/gcc/testsuite/g++.dg/cpp0x/alias-decl-16.C
> +++ b/gcc/testsuite/g++.dg/cpp0x/alias-decl-16.C
> @@ -23,6 +23,6 @@ template<class T>
>  using A3 =
>      enum B3 {b = 0;}; //{ dg-error "types may not be defined in alias template" }
>
> -A3<int> a3;
> +A3<int> a3; // { dg-error "'A3' does not name a type" }
>
>  int main() { }
> diff --git a/gcc/testsuite/g++.dg/cpp0x/alias-decl-28.C b/gcc/testsuite/g++.dg/cpp0x/alias-decl-28.C
> new file mode 100644
> index 0000000..086b5e5
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp0x/alias-decl-28.C
> @@ -0,0 +1,7 @@
> +// Origin: PR c++/54401
> +// { dg-do compile { target c++11 } }
> +
> +template<typename>
> +struct X {
> +    using type = T; // { dg-error "expected type-specifier|does not name a type" }
> +};
> --
> 1.7.11.7
>
>
> --
>                 Dodji

Sound good to me (except for the comment).
thanks!

-- Gaby
Jason Merrill - Dec. 7, 2012, 2:43 p.m.
On 12/07/2012 08:42 AM, Gabriel Dos Reis wrote:
> Sound good to me (except for the comment).

Me, too.

Jason

Patch

diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
index a010f1f..c39ef30 100644
--- a/gcc/cp/parser.c
+++ b/gcc/cp/parser.c
@@ -15262,7 +15262,11 @@  cp_parser_using_declaration (cp_parser* parser,
 /* Parse an alias-declaration.
 
    alias-declaration:
-     using identifier attribute-specifier-seq [opt] = type-id  */
+     using identifier attribute-specifier-seq [opt] = type-id
+
+   Note that if this function is used in the context of a tentative
+   parse, it commits the currently active tentative parse after it
+   sees the '=' token.  */
 
 static tree
 cp_parser_alias_declaration (cp_parser* parser)
@@ -15295,6 +15299,8 @@  cp_parser_alias_declaration (cp_parser* parser)
   if (cp_parser_error_occurred (parser))
     return error_mark_node;
 
+  cp_parser_commit_to_tentative_parse (parser);
+
   /* Now we are going to parse the type-id of the declaration.  */
 
   /*
@@ -15324,10 +15330,19 @@  cp_parser_alias_declaration (cp_parser* parser)
   if (parser->num_template_parameter_lists)
     parser->type_definition_forbidden_message = saved_message;
 
+  if (type == error_mark_node)
+    {
+      cp_parser_skip_to_end_of_block_or_statement (parser);
+      return error_mark_node;
+    }
+
   cp_parser_require (parser, CPP_SEMICOLON, RT_SEMICOLON);
 
   if (cp_parser_error_occurred (parser))
-    return error_mark_node;
+    {
+      cp_parser_skip_to_end_of_block_or_statement (parser);
+      return error_mark_node;
+    }
 
   /* A typedef-name can also be introduced by an alias-declaration. The
      identifier following the using keyword becomes a typedef-name. It has
@@ -19063,9 +19078,19 @@  cp_parser_member_declaration (cp_parser* parser)
       else
 	{
 	  tree decl;
+	  bool alias_decl_expected;
 	  cp_parser_parse_tentatively (parser);
 	  decl = cp_parser_alias_declaration (parser);
-	  if (cp_parser_parse_definitely (parser))
+	  /* Note that if we actually see the '=' token after the
+	     identifier, cp_parser_alias_declaration commits the
+	     tentative parse.  In that case, we really expects an
+	     alias-declaration.  Otherwise, we expect a using
+	     declaration.  */
+	  alias_decl_expected =
+	    !cp_parser_uncommitted_to_tentative_parse_p (parser);
+	  cp_parser_parse_definitely (parser);
+
+	  if (alias_decl_expected)
 	    finish_member_declaration (decl);
 	  else
 	    cp_parser_using_declaration (parser,
diff --git a/gcc/testsuite/g++.dg/cpp0x/alias-decl-16.C b/gcc/testsuite/g++.dg/cpp0x/alias-decl-16.C
index d66660a..ce6ad0a 100644
--- a/gcc/testsuite/g++.dg/cpp0x/alias-decl-16.C
+++ b/gcc/testsuite/g++.dg/cpp0x/alias-decl-16.C
@@ -23,6 +23,6 @@  template<class T>
 using A3 =
     enum B3 {b = 0;}; //{ dg-error "types may not be defined in alias template" }
 
-A3<int> a3;
+A3<int> a3; // { dg-error "'A3' does not name a type" }
 
 int main() { }
diff --git a/gcc/testsuite/g++.dg/cpp0x/alias-decl-28.C b/gcc/testsuite/g++.dg/cpp0x/alias-decl-28.C
new file mode 100644
index 0000000..086b5e5
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp0x/alias-decl-28.C
@@ -0,0 +1,7 @@ 
+// Origin: PR c++/54401
+// { dg-do compile { target c++11 } }
+
+template<typename>
+struct X {
+    using type = T; // { dg-error "expected type-specifier|does not name a type" }
+};