This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [C++0x] avoid extra tentative parse in range-based for loops
On Tue, Nov 23, 2010 at 9:24 PM, Jason Merrill <jason@redhat.com> wrote:
> 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. Â:)
Right. Then, I'll go with the first and the second ones.
>> 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.
Then, what about adding somewhere?
TREE_USED (range_decl) = 1;
DECL_READ_P (range_decl) = 1;
This way, the users wouldn't get bothered with these warnings that
they cannot avoid easily.
>> + Â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.
That was my first option but sadly it doesn't work. The problem is
that cp_parser_expression_statement must be called after
begin_for_stmt. And in my patch cp_parser_for_init_statement doesn't
call begin_for_stmt anymore.
Sure, you could reorder it in many other ways, moving pieces of code
from/to begin_for_scope/begin_{,range_}for_stmt and the cp_parser_for*
functions, but this seemed the simplest to me.
Maybe a few comments here and there are welcome.
>> + Â 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.
Ok. There are a few other comments totally wrong, also. I'll correct them.
>> - Â Â Â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.
English is such an imprecise language! (my use of it at least). What I
meant is what the code does.
What about something like this?:
If JUST_ONE_DECLARATOR is not NULL, the pointed tree will be set to the
parsed declaration iff it is a single declarator without initializer.
If this is the case, the
trailing `;', if present, will not be checked; in either way the
ending token will not be consumed.
>Â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?
Yes, we can. Basically, the difference would be in code like:
for (int a, b, c ! x)
My code gives: expected initializer before '!'.
Yours gives: expected ';' before '!'.
I don't have a strong preference for either, but I was just following
the 'least change in behavior' rule.
>> + Â Â Â Â 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.
Actually, not so simple. The condition (is_initialized &&
initialization_kind == CPP_EOF) is TRUE when (just_one_declarator &&
*just_one_declarator != error_mark_node) and an unknown token is seen
instead of the initializer. It has a triple effect:
* Do not issue the error "expected initializer".
* Pass SD_INITIALIZED to start_decl.
* Do not parse the initializer, since it is not here.
* Do not call cp_finish_decl, since it will be called by the upper function.
When a known token is seen, (*just_one_declarator) is set to
error_mark_node, so this two conditions should be equal after that:
(just_one_declarator && *just_one_declarator != error_mark_node)
(is_initialized && initialization_kind == CPP_EOF)
These conditions could be interchanged, but all the tests are
necessary. You may want to add a boolean local variable with this same
value to make the conditions simpler, but I don't know if it is worth
it.
Sure, a few comments explaining this would be nice...
If you agree that this is the right way, I will write a new patch with
your suggestions and your answers to this message.
Regards.
--
Rodrigo