Patchwork [C++] PR 51908

login
register
mail settings
Submitter Paolo Carlini
Date June 5, 2013, 12:34 a.m.
Message ID <51AE8778.8080901@oracle.com>
Download mbox | patch
Permalink /patch/248876/
State New
Headers show

Comments

Paolo Carlini - June 5, 2013, 12:34 a.m.
Hi,

this ICE on valid happens in the cp_parser_abort_tentative_parse called 
at the end of cp_parser_decltype_expr. I started seriously looking into 
it when I noticed that a variant of the testcase not using variadic 
templates is fine, thus I concentrated on 
cp_parser_parameter_declaration and noticed an inconsistency in the 
following comment and the code implementing it:

        /* After seeing a decl-specifier-seq, if the next token is not a
-     "(", there is no possibility that the code is a valid
+     "(" nor '...', there is no possibility that the code is a valid
       expression.  Therefore, if parsing tentatively, we commit at
       this point.  */

clearly in the case at issue of decl-specifier-seq followed by ellipsis 
we may be parsing a perfectly valid declaration.

Tested x86_64-linux, as usual.

Thanks,
Paolo.

////////////////////////
/cp
2013-06-05  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/51908
	* parser.c (cp_parser_parameter_declaration): Do not commit to
	tentative parse after a decl-specifier-seq followed by ellipsis.

/testsuite
2013-06-05  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/51908
	* g++.dg/cpp0x/variadic144.C: New.
Jason Merrill - June 5, 2013, 12:53 p.m.
On 06/04/2013 08:34 PM, Paolo Carlini wrote:
>         /* After seeing a decl-specifier-seq, if the next token is not a
> -     "(", there is no possibility that the code is a valid
> +     "(" nor '...', there is no possibility that the code is a valid
>        expression.  Therefore, if parsing tentatively, we commit at
>        this point.  */
>
> clearly in the case at issue of decl-specifier-seq followed by ellipsis
> we may be parsing a perfectly valid declaration.

Yes, but not an expression, which is why we are committing there: we 
know it *has* to be a declaration.  The non-variadic case should hit 
that commit, so your change doesn't seem to make the variadic case more 
consistent.

Jason
Paolo Carlini - June 5, 2013, 1:07 p.m.
Hi,

On 06/05/2013 02:53 PM, Jason Merrill wrote:
> On 06/04/2013 08:34 PM, Paolo Carlini wrote:
>>         /* After seeing a decl-specifier-seq, if the next token is not a
>> -     "(", there is no possibility that the code is a valid
>> +     "(" nor '...', there is no possibility that the code is a valid
>>        expression.  Therefore, if parsing tentatively, we commit at
>>        this point.  */
>>
>> clearly in the case at issue of decl-specifier-seq followed by ellipsis
>> we may be parsing a perfectly valid declaration.
>
> Yes, but not an expression, which is why we are committing there: we 
> know it *has* to be a declaration.
I see, I didn't notice the reference to *expressions* in the comment.
>   The non-variadic case should hit that commit, so your change doesn't 
> seem to make the variadic case more consistent.
But the non-variadic case does *not* hit the commit, because is handled 
above:

   /* If the next token is a `)', `,', `=', `>', or `...', then there
      is no declarator. However, when variadic templates are enabled,
      there may be a declarator following `...'.  */
   if (token->type == CPP_CLOSE_PAREN
       || token->type == CPP_COMMA
       || token->type == CPP_EQ
       || token->type == CPP_GREATER)
     {
       declarator = NULL;
       if (parenthesized_p)
     *parenthesized_p = false;
     }

Thus it seems to me that the variadic case should also somehow not-hit 
the commit, do you agree?

Paolo.
Jason Merrill - June 5, 2013, 1:19 p.m.
On 06/05/2013 09:07 AM, Paolo Carlini wrote:
> Hi,
>
> On 06/05/2013 02:53 PM, Jason Merrill wrote:
>> On 06/04/2013 08:34 PM, Paolo Carlini wrote:
>>>         /* After seeing a decl-specifier-seq, if the next token is not a
>>> -     "(", there is no possibility that the code is a valid
>>> +     "(" nor '...', there is no possibility that the code is a valid
>>>        expression.  Therefore, if parsing tentatively, we commit at
>>>        this point.  */
>>>
>>> clearly in the case at issue of decl-specifier-seq followed by ellipsis
>>> we may be parsing a perfectly valid declaration.
>>
>> Yes, but not an expression, which is why we are committing there: we
>> know it *has* to be a declaration.
> I see, I didn't notice the reference to *expressions* in the comment.
>>   The non-variadic case should hit that commit, so your change doesn't
>> seem to make the variadic case more consistent.
> But the non-variadic case does *not* hit the commit, because is handled
> above:
>
>    /* If the next token is a `)', `,', `=', `>', or `...', then there
>       is no declarator. However, when variadic templates are enabled,
>       there may be a declarator following `...'.  */
>    if (token->type == CPP_CLOSE_PAREN
>        || token->type == CPP_COMMA
>        || token->type == CPP_EQ
>        || token->type == CPP_GREATER)
>      {
>        declarator = NULL;
>        if (parenthesized_p)
>      *parenthesized_p = false;
>      }
>
> Thus it seems to me that the variadic case should also somehow not-hit
> the commit, do you agree?

Well, the comment here is correct: (Args... args) would also be 
well-formed.  But in the testcase there isn't a declarator, so the 
comment following the above, "Otherwise, there should be a declarator", 
is wrong.  I guess the above should also check for ... followed by one 
of those tokens.

In any case, the commit doesn't seem like the problem.  I'm not sure why 
it's in the else block rather than before the if.

Jason
Paolo Carlini - June 5, 2013, 1:45 p.m.
.. true that my first try didn't fix this non-variadic variant, because 
we have a declarator and again we hit the commit:

struct foo
{
   template <typename Ret, typename Args>
   operator decltype(static_cast<Ret(*)(Args args)>(0)) () const;
};

Paolo.
Jason Merrill - June 5, 2013, 2:19 p.m.
On 06/05/2013 09:19 AM, Jason Merrill wrote:
> In any case, the commit doesn't seem like the problem.

Oh, I see, of course it is.  The problem is that it commits to all 
levels, so that if we happen to be in a nested tentative parse we can't 
commit to the inner one without committing to the outer one, which might 
not be appropriate.  This seems like a basic flaw in our tentative 
parsing mechanism.

What happens if we disable that commit entirely?

Jason

Patch

Index: cp/parser.c
===================================================================
--- cp/parser.c	(revision 199675)
+++ cp/parser.c	(working copy)
@@ -17887,7 +17887,7 @@  cp_parser_parameter_declaration (cp_parser *parser
       parser->default_arg_ok_p = false;
 
       /* After seeing a decl-specifier-seq, if the next token is not a
-	 "(", there is no possibility that the code is a valid
+	 "(" nor '...', there is no possibility that the code is a valid
 	 expression.  Therefore, if parsing tentatively, we commit at
 	 this point.  */
       if (!parser->in_template_argument_list_p
@@ -17901,7 +17901,8 @@  cp_parser_parameter_declaration (cp_parser *parser
 	  && !parser->in_type_id_in_expr_p
 	  && cp_parser_uncommitted_to_tentative_parse_p (parser)
 	  && cp_lexer_next_token_is_not (parser->lexer, CPP_OPEN_BRACE)
-	  && cp_lexer_next_token_is_not (parser->lexer, CPP_OPEN_PAREN))
+	  && cp_lexer_next_token_is_not (parser->lexer, CPP_OPEN_PAREN)
+	  && cp_lexer_next_token_is_not (parser->lexer, CPP_ELLIPSIS))
 	cp_parser_commit_to_tentative_parse (parser);
       /* Parse the declarator.  */
       declarator_token_start = token;
Index: testsuite/g++.dg/cpp0x/variadic144.C
===================================================================
--- testsuite/g++.dg/cpp0x/variadic144.C	(revision 0)
+++ testsuite/g++.dg/cpp0x/variadic144.C	(working copy)
@@ -0,0 +1,8 @@ 
+// PR c++/51908
+// { dg-do compile { target c++11 } }
+
+struct foo
+{
+  template <typename Ret, typename... Args>
+  operator decltype(static_cast<Ret (*)(Args...)>(nullptr)) () const;
+};