Re: [C++0x] avoid extra tentative parse in range-based for loops

[Jason Merrill <jason at redhat dot com>]

On 11/15/2010 05:34 PM, Rodrigo Rivas wrote:
> Should I write a testcase for this? I find difficult to write one that
> looks minimally realistic:
>      for (struct S {} *s : { (S*)NULL, (S*)NULL, (S*)NULL }) ;
> or:
>      for (struct S {} s : { S(), S(), S() }) ;
> or even:
>      for (struct S { S(int xx):x(xx){} int x; } s : { 1, 2, 3 }) ;
> are correct but mostly useless.

Testcases don't need to be useful.  :)

> By the way, now that we are changing things, I find somewhat
> surprising that the name of the declarator is mandatory.  Let's say
> that I want to repeat a identical code twice:
>      for (int x : {0, 0})
>      { /*...*/ }
>
> But this issues: warning: variable 'x' set but not used
> [-Wunused-but-set-variable].
>
> Shouldn't this be allowed, then?
>      for (int : {0, 0})
>      { /*...*/ }

Sure, that seems reasonable, but I think too late now.

> +  if (kind == FOR_LOOP_EXPRESSION)
> +    cp_parser_expression_statement (parser, NULL_TREE);

Why did this move here?  It seems to fit better in 
cp_parser_for_init_statement, where it was before.  And then you don't 
need to distinguish between _DECLARATION and _EXPRESSION.

> +   If JUST_ONE_DECLARATOR is not NULL, the pointed tree will be set to the
> +   parsed declaration iff it is a single declarator. In either way, the
> +   trailing `;', if present, will not be checked nor consumed.  */

This should also say that it's set to NULL_TREE or error_mark_node if 0 
or >1 declarators are seen.  And document that it's used by 
cp_parser_for_init_statement.

> -      else if (token->type == CPP_SEMICOLON)
> +      else if (token->type == CPP_SEMICOLON
> +              || (just_one_declarator
> +                  && *just_one_declarator != error_mark_node))

But here you're checking the trailing ; if more than one declarator is 
seen, which seems to contradict the comment.  Can't we just say it's OK 
if just_one_declarator is set, and let cp_parser_for_init_statement 
complain if we see something unsuitable?

> +         if (just_one_declarator && *just_one_declarator != error_mark_node)
> +           {
> +             is_initialized = SD_INITIALIZED;
> +           }
...
> +  if (is_initialized && initialization_kind != CPP_EOF)
...
> +  if (!friend_p && decl && decl != error_mark_node
> +      && (!is_initialized || initialization_kind != CPP_EOF))

It seems that the purpose of this code is to pass SD_INITIALIZED into 
start_decl; I think it would be clearer to check just_one_declarator 
just before the call to start_decl, with a comment about why.

Jason