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

login
register
mail settings
Submitter Rodrigo Rivas
Date Nov. 15, 2010, 10:34 p.m.
Message ID <AANLkTimTvP7X=Z8FEL-ogbkEWt9UpttiDBs+wXy768+Y@mail.gmail.com>
Download mbox | patch
Permalink /patch/71311/
State New
Headers show

Comments

Rodrigo Rivas - Nov. 15, 2010, 10:34 p.m.
On Mon, Nov 15, 2010 at 3:27 PM, Jason Merrill <jason@redhat.com> wrote:
> At the standards committee meeting last week I proposed changing
> type-specifier-seq to decl-specifier-seq in the range-based for loop
> (http://open-std.org/jtc1/sc22/wg21/docs/cwg_active.html#1204), and people
> agreed.  The change hasn't gone into the WP yet, but I expect it will at the
> next meeting.  Can you update your patch accordingly?

Sure. Actually I was parsing the range-for using
cp_parser_decl_specifier_seq and then checking whether it is really a
type-specifier-seq. Just removing this check should do the trick.

> Looking back, it looks like I didn't mention that I was going to suggest
> this; sorry for not keeping you in the loop.
No problem.

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.

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})
    { /*...*/ }

Or a more useful example:

    struct Work
    {
        Work(const char *name)
        {
            /* do something useful */
        }
    };
    const char *names[] = {"foo", "bar", "goo"};
    /*...*/
    for (Work : names)
        ;

Regards.
Rodrigo
Jason Merrill - Nov. 23, 2010, 8:24 p.m.
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
Rodrigo Rivas - Nov. 25, 2010, 9:46 a.m.
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
Jason Merrill - Nov. 26, 2010, 4:20 p.m.
On 11/25/2010 04:46 AM, Rodrigo Rivas wrote:
>>> But this issues: warning: variable 'x' set but not used
>>> [-Wunused-but-set-variable].
>
> Then, what about adding somewhere?
> TREE_USED (range_decl) = 1;
> DECL_READ_P (range_decl) = 1;

Sounds good.

>>> +  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.

Why?  You moved the scope stuff into begin_for_scope, which is called 
before cp_parser_for_init_statement.

>>> +   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.
>
> 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.

That still doesn't cover what happens to *just_one_declarator in the 
other cases.

>> 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.

I don't think either message is significantly better, so I'd go with the 
simpler logic.

>>> +         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.

Right.  Not setting is_initialized covers #3, and I think the others 
should be handled by a separate flag instead of special values of the 
initialization variables.

> 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.

I think it's definitely worth it; that's a lot clearer than the magic 
CPP_EOF.  Also, let's rename just_one_declarator to maybe_range_for_decl 
to make it clearer what's going on.

Jason
Paolo Bonzini - Jan. 13, 2011, 9:04 a.m.
On 11/26/2010 05:20 PM, Jason Merrill wrote:
>>
>> Then, what about adding somewhere?
>> TREE_USED (range_decl) = 1;
>> DECL_READ_P (range_decl) = 1;
>
> Sounds good.

Replying to an old message: does __attribute__ ((__unused__)) work in 
this context?  Or also, "for (Work : { w(), w(), w() })" could be 
accepted as a GCC extension.

Paolo
Rodrigo Rivas - Jan. 13, 2011, 11:24 a.m.
On Thu, Jan 13, 2011 at 10:04 AM, Paolo Bonzini <bonzini@gnu.org> wrote:
>>> Then, what about adding somewhere?
>>> TREE_USED (range_decl) = 1;
>>> DECL_READ_P (range_decl) = 1;
>
> Replying to an old message: does __attribute__ ((__unused__)) work in this
> context?  Or also, "for (Work : { w(), w(), w() })" could be accepted as a
> GCC extension.

Actually, it does work! If I remove these two lines, then the code:
      for (int a __attribute__((unused)): { 1, 2, 3 }) { }
gives no warning at all with -Wall.

So, maybe that two lines are unnecessary after all. And the same goes
for the nameless-variable extension.

Rodrigo
Paolo Bonzini - Jan. 13, 2011, 11:32 a.m.
On 01/13/2011 12:24 PM, Rodrigo Rivas wrote:
> On Thu, Jan 13, 2011 at 10:04 AM, Paolo Bonzini<bonzini@gnu.org>  wrote:
>>>> Then, what about adding somewhere?
>>>> TREE_USED (range_decl) = 1;
>>>> DECL_READ_P (range_decl) = 1;
>>
>> Replying to an old message: does __attribute__ ((__unused__)) work in this
>> context?  Or also, "for (Work : { w(), w(), w() })" could be accepted as a
>> GCC extension.
>
> Actually, it does work! If I remove these two lines, then the code:
>        for (int a __attribute__((unused)): { 1, 2, 3 }) { }
> gives no warning at all with -Wall.

Yeah, it's just a declaration.  I was only concerned about the 
subtleties of the parser and whether it accepted attributes at all.

> So, maybe that two lines are unnecessary after all.

Yes.

> And the same goes for the nameless-variable extension.

It could still be useful to propose it to the committee, though Jason 
said it is late for C++0x.

Paolo

Patch

diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
index 67f4f93..efac88d 100644
--- a/gcc/cp/cp-tree.h
+++ b/gcc/cp/cp-tree.h
@@ -5221,12 +5221,13 @@  extern tree begin_do_stmt			(void);
 extern void finish_do_body			(tree);
 extern void finish_do_stmt			(tree, tree);
 extern tree finish_return_stmt			(tree);
-extern tree begin_for_stmt			(void);
+extern tree begin_for_scope			(tree *);
+extern tree begin_for_stmt			(tree, tree);
 extern void finish_for_init_stmt		(tree);
 extern void finish_for_cond			(tree, tree);
 extern void finish_for_expr			(tree, tree);
 extern void finish_for_stmt			(tree);
-extern tree begin_range_for_stmt		(void);
+extern tree begin_range_for_stmt		(tree, tree);
 extern void finish_range_for_decl		(tree, tree, tree);
 extern void finish_range_for_stmt		(tree);
 extern tree finish_break_stmt			(void);
diff --git a/gcc/cp/init.c b/gcc/cp/init.c
index 670c7a5..003eb01 100644
--- a/gcc/cp/init.c
+++ b/gcc/cp/init.c
@@ -3108,7 +3108,7 @@  build_vec_init (tree base, tree maxindex, tree init,
       tree elt_init;
       tree to;
 
-      for_stmt = begin_for_stmt ();
+      for_stmt = begin_for_stmt (NULL_TREE, NULL_TREE);
       finish_for_init_stmt (for_stmt);
       finish_for_cond (build2 (NE_EXPR, boolean_type_node, iterator,
 			       build_int_cst (TREE_TYPE (iterator), -1)),
diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
index 906b0c3..1c784bb 100644
--- a/gcc/cp/parser.c
+++ b/gcc/cp/parser.c
@@ -270,6 +270,12 @@  typedef enum required_token {
   RT_CLASS_TYPENAME_TEMPLATE /* class, typename, or template */
 } required_token;
 
+typedef enum kind_of_for_loop
+{
+  FOR_LOOP_EXPRESSION,
+  FOR_LOOP_DECLARATION,
+  FOR_LOOP_RANGE
+} kind_of_for_loop;
 /* Prototypes.  */
 
 static cp_lexer *cp_lexer_new_main
@@ -1850,12 +1856,14 @@  static tree cp_parser_condition
   (cp_parser *);
 static tree cp_parser_iteration_statement
   (cp_parser *);
-static void cp_parser_for_init_statement
-  (cp_parser *);
-static tree  cp_parser_c_for
-  (cp_parser *);
-static tree  cp_parser_range_for
+static kind_of_for_loop cp_parser_for_init_statement
+  (cp_parser *, tree *decl);
+static tree cp_parser_for
   (cp_parser *);
+static tree cp_parser_c_for
+  (cp_parser *, tree, tree, kind_of_for_loop);
+static tree cp_parser_range_for
+  (cp_parser *, tree, tree, tree);
 static tree cp_parser_jump_statement
   (cp_parser *);
 static void cp_parser_declaration_statement
@@ -1875,7 +1883,7 @@  static void cp_parser_declaration
 static void cp_parser_block_declaration
   (cp_parser *, bool);
 static void cp_parser_simple_declaration
-  (cp_parser *, bool);
+  (cp_parser *, bool, tree *);
 static void cp_parser_decl_specifier_seq
   (cp_parser *, cp_parser_flags, cp_decl_specifier_seq *, int *);
 static tree cp_parser_storage_class_specifier_opt
@@ -1925,7 +1933,7 @@  static tree cp_parser_decltype
 /* Declarators [gram.dcl.decl] */
 
 static tree cp_parser_init_declarator
-  (cp_parser *, cp_decl_specifier_seq *, VEC (deferred_access_check,gc)*, bool, bool, int, bool *);
+  (cp_parser *, cp_decl_specifier_seq *, VEC (deferred_access_check,gc)*, bool, bool, int, bool *, tree *);
 static cp_declarator *cp_parser_declarator
   (cp_parser *, cp_parser_declarator_kind, int *, bool *, bool);
 static cp_declarator *cp_parser_direct_declarator
@@ -8652,20 +8660,38 @@  cp_parser_condition (cp_parser* parser)
 /* Parses a traditional for-statement until the closing ')', not included. */
 
 static tree
-cp_parser_c_for (cp_parser *parser)
+cp_parser_for (cp_parser *parser)
+{
+  tree init, scope, decl;
+  kind_of_for_loop kind;
+
+  /* Begin the for-statement.  */
+  scope = begin_for_scope (&init);
+
+  /* Parse the initialization.  */
+  kind = cp_parser_for_init_statement (parser, &decl);
+
+  if (kind == FOR_LOOP_RANGE)
+    return cp_parser_range_for (parser, scope, init, decl);
+  else
+    return cp_parser_c_for (parser, scope, init, kind);
+}
+
+static tree
+cp_parser_c_for (cp_parser *parser, tree scope, tree init,
+		 kind_of_for_loop kind)
 {
   /* Normal for loop */
-  tree stmt;
   tree condition = NULL_TREE;
   tree expression = NULL_TREE;
+  tree stmt;
 
-  /* Begin the for-statement.  */
-  stmt = begin_for_stmt ();
+  stmt = begin_for_stmt (scope, init);
 
-  /* Parse the initialization.  */
-  cp_parser_for_init_statement (parser);
-  finish_for_init_stmt (stmt);
+  if (kind == FOR_LOOP_EXPRESSION)
+    cp_parser_expression_statement (parser, NULL_TREE);
 
+  finish_for_init_stmt (stmt);
   /* If there's a condition, process it.  */
   if (cp_lexer_next_token_is_not (parser->lexer, CPP_SEMICOLON))
     condition = cp_parser_condition (parser);
@@ -8684,7 +8710,7 @@  cp_parser_c_for (cp_parser *parser)
 /* Tries to parse a range-based for-statement:
 
   range-based-for:
-    type-specifier-seq declarator : expression
+    decl-specifier-seq declarator : expression
 
   If succesful, assigns to *DECL the DECLARATOR and to *EXPR the
   expression. Note that the *DECL is returned unfinished, so
@@ -8693,48 +8719,10 @@  cp_parser_c_for (cp_parser *parser)
   Returns TRUE iff a range-based for is parsed. */
 
 static tree
-cp_parser_range_for (cp_parser *parser)
+cp_parser_range_for (cp_parser *parser, tree scope, tree init, tree range_decl)
 {
-  tree stmt, range_decl, range_expr;
-  cp_decl_specifier_seq type_specifiers;
-  cp_declarator *declarator;
-  const char *saved_message;
-  tree attributes, pushed_scope;
+  tree stmt, range_expr;
 
-  cp_parser_parse_tentatively (parser);
-  /* New types are not allowed in the type-specifier-seq for a
-     range-based for loop.  */
-  saved_message = parser->type_definition_forbidden_message;
-  parser->type_definition_forbidden_message
-    = G_("types may not be defined in range-based for loops");
-  /* Parse the type-specifier-seq.  */
-  cp_parser_type_specifier_seq (parser, /*is_declaration==*/true,
-				/*is_trailing_return=*/false,
-				&type_specifiers);
-  /* Restore the saved message.  */
-  parser->type_definition_forbidden_message = saved_message;
-  /* If all is well, we might be looking at a declaration.  */
-  if (cp_parser_error_occurred (parser))
-    {
-      cp_parser_abort_tentative_parse (parser);
-      return NULL_TREE;
-    }
-  /* Parse the declarator.  */
-  declarator = cp_parser_declarator (parser, CP_PARSER_DECLARATOR_NAMED,
-				     /*ctor_dtor_or_conv_p=*/NULL,
-				     /*parenthesized_p=*/NULL,
-				     /*member_p=*/false);
-  /* Parse the attributes.  */
-  attributes = cp_parser_attributes_opt (parser);
-  /* The next token should be `:'. */
-  if (cp_lexer_next_token_is_not (parser->lexer, CPP_COLON))
-    cp_parser_simulate_error (parser);
-
-  /* Check if it is a range-based for */
-  if (!cp_parser_parse_definitely (parser))
-    return NULL_TREE;
-
-  cp_parser_require (parser, CPP_COLON, RT_COLON);
   if (cp_lexer_next_token_is (parser->lexer, CPP_OPEN_BRACE))
     {
       bool expr_non_constant_p;
@@ -8746,23 +8734,16 @@  cp_parser_range_for (cp_parser *parser)
   /* If in template, STMT is converted to a normal for-statements
      at instantiation. If not, it is done just ahead. */
   if (processing_template_decl)
-    stmt = begin_range_for_stmt ();
-  else
-    stmt = begin_for_stmt ();
-
-  /* Create the declaration. It must be after begin{,_range}_for_stmt(). */
-  range_decl = start_decl (declarator, &type_specifiers,
-			   /*initialized_p=*/SD_INITIALIZED,
-			   attributes, /*prefix_attributes=*/NULL_TREE,
-			   &pushed_scope);
-  /* No scope allowed here */
-  pop_scope (pushed_scope);
-
-  if (TREE_CODE (stmt) == RANGE_FOR_STMT)
-    finish_range_for_decl (stmt, range_decl, range_expr);
+    {
+      stmt = begin_range_for_stmt (scope, init);
+      finish_range_for_decl (stmt, range_decl, range_expr);
+    }
   else
-    /* Convert the range-based for loop into a normal for-statement. */
-    stmt = cp_convert_range_for (stmt, range_decl, range_expr);
+    {
+      stmt = begin_for_stmt (scope, init);
+      /* Convert the range-based for loop into a normal for-statement. */
+      stmt = cp_convert_range_for (stmt, range_decl, range_expr);
+    }
 
   return stmt;
 }
@@ -8989,12 +8970,7 @@  cp_parser_iteration_statement (cp_parser* parser)
 	/* Look for the `('.  */
 	cp_parser_require (parser, CPP_OPEN_PAREN, RT_OPEN_PAREN);
 
-	if (cxx_dialect == cxx0x)
-	  statement = cp_parser_range_for (parser);
-	else
-	  statement = NULL_TREE;
-	if (statement == NULL_TREE)
-	  statement = cp_parser_c_for (parser);
+	statement = cp_parser_for (parser);
 
 	/* Look for the `)'.  */
 	cp_parser_require (parser, CPP_CLOSE_PAREN, RT_CLOSE_PAREN);
@@ -9024,8 +9000,8 @@  cp_parser_iteration_statement (cp_parser* parser)
      expression-statement
      simple-declaration  */
 
-static void
-cp_parser_for_init_statement (cp_parser* parser)
+static kind_of_for_loop
+cp_parser_for_init_statement (cp_parser* parser, tree *decl)
 {
   /* If the next token is a `;', then we have an empty
      expression-statement.  Grammatically, this is also a
@@ -9035,19 +9011,44 @@  cp_parser_for_init_statement (cp_parser* parser)
      declaration.  */
   if (cp_lexer_next_token_is_not (parser->lexer, CPP_SEMICOLON))
     {
+      kind_of_for_loop res = FOR_LOOP_DECLARATION;
       /* We're going to speculatively look for a declaration, falling back
 	 to an expression, if necessary.  */
       cp_parser_parse_tentatively (parser);
       /* Parse the declaration.  */
       cp_parser_simple_declaration (parser,
-				    /*function_definition_allowed_p=*/false);
+				    /*function_definition_allowed_p=*/false,
+				    decl);
       /* If the tentative parse failed, then we shall need to look for an
 	 expression-statement.  */
+      if (cp_lexer_next_token_is (parser->lexer, CPP_COLON))
+	{
+	  if (cxx_dialect < cxx0x)
+	    {
+	      location_t loc = cp_lexer_peek_token (parser->lexer)->location;
+	      error_at (loc, "range-based-for loops are not allowed "
+			"in C++98 mode");
+	      cp_parser_skip_to_closing_parenthesis (parser,
+						     /*recovering=*/true,
+						     /*or_comma=*/false,
+						     /*consume_paren=*/false);
+	    }
+	  else
+	    {
+	      /* Consume the ':' */
+	      cp_lexer_consume_token (parser->lexer);
+	      res = FOR_LOOP_RANGE;
+	    }
+	}
+      else
+	/* The ';' is not consumed yet because we told
+	   cp_parser_simple_declaration not to.  */
+        cp_parser_require (parser, CPP_SEMICOLON, RT_SEMICOLON);
+
       if (cp_parser_parse_definitely (parser))
-	return;
+	return res;
     }
-
-  cp_parser_expression_statement (parser, NULL_TREE);
+  return FOR_LOOP_EXPRESSION;
 }
 
 /* Parse a jump-statement.
@@ -9534,7 +9535,8 @@  cp_parser_block_declaration (cp_parser *parser,
     cp_parser_static_assert (parser, /*member_p=*/false);
   /* Anything else must be a simple-declaration.  */
   else
-    cp_parser_simple_declaration (parser, !statement_p);
+    cp_parser_simple_declaration (parser, !statement_p,
+				  /*just_one_declarator*/NULL);
 }
 
 /* Parse a simple-declaration.
@@ -9547,16 +9549,24 @@  cp_parser_block_declaration (cp_parser *parser,
      init-declarator-list , init-declarator
 
    If FUNCTION_DEFINITION_ALLOWED_P is TRUE, then we also recognize a
-   function-definition as a simple-declaration.  */
+   function-definition as a simple-declaration.
+
+   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.  */
 
 static void
 cp_parser_simple_declaration (cp_parser* parser,
-			      bool function_definition_allowed_p)
+			      bool function_definition_allowed_p,
+			      tree *just_one_declarator)
 {
   cp_decl_specifier_seq decl_specifiers;
   int declares_class_or_enum;
   bool saw_declarator;
 
+  if (just_one_declarator)
+    *just_one_declarator = NULL_TREE;
+
   /* Defer access checks until we know what is being declared; the
      checks for names appearing in the decl-specifier-seq should be
      done as if we were in the scope of the thing being declared.  */
@@ -9631,6 +9641,8 @@  cp_parser_simple_declaration (cp_parser* parser,
 	  token = cp_lexer_peek_token (parser->lexer);
 	  gcc_assert (token->type == CPP_COMMA);
 	  cp_lexer_consume_token (parser->lexer);
+	  if (just_one_declarator)
+	    *just_one_declarator = error_mark_node;
 	}
       else
 	saw_declarator = true;
@@ -9641,7 +9653,8 @@  cp_parser_simple_declaration (cp_parser* parser,
 					function_definition_allowed_p,
 					/*member_p=*/false,
 					declares_class_or_enum,
-					&function_definition_p);
+					&function_definition_p,
+					just_one_declarator);
       /* If an error occurred while parsing tentatively, exit quickly.
 	 (That usually happens when in the body of a function; each
 	 statement is treated as a declaration-statement until proven
@@ -9671,13 +9684,17 @@  cp_parser_simple_declaration (cp_parser* parser,
 	      return;
 	    }
 	}
+      if (just_one_declarator && *just_one_declarator == NULL_TREE)
+	*just_one_declarator = decl;
       /* The next token should be either a `,' or a `;'.  */
       token = cp_lexer_peek_token (parser->lexer);
       /* If it's a `,', there are more declarators to come.  */
       if (token->type == CPP_COMMA)
 	/* will be consumed next time around */;
       /* If it's a `;', we are done.  */
-      else if (token->type == CPP_SEMICOLON)
+      else if (token->type == CPP_SEMICOLON
+	       || (just_one_declarator
+		   && *just_one_declarator != error_mark_node))
 	break;
       /* Anything else is an error.  */
       else
@@ -9715,7 +9732,8 @@  cp_parser_simple_declaration (cp_parser* parser,
     }
 
   /* Consume the `;'.  */
-  cp_parser_require (parser, CPP_SEMICOLON, RT_SEMICOLON);
+  if (!just_one_declarator)
+      cp_parser_require (parser, CPP_SEMICOLON, RT_SEMICOLON);
 
  done:
   pop_deferring_access_checks ();
@@ -14319,7 +14337,13 @@  cp_parser_asm_definition (cp_parser* parser)
    have been completely parsed.
 
    FUNCTION_DEFINITION_P may be NULL if FUNCTION_DEFINITION_ALLOWED_P
-   is FALSE.  */
+   is FALSE.
+
+   If JUST_ONE_DECLARATOR is not NULL, the pointed tree will be set to the
+   parsed declaration iff it is a single declarator without initialization.
+   The trailing ';' if present will not be checked nor consumed.
+   If returned, the declarator will be created with SD_INITIALIZED but
+   will not call cp_finish_decl.  */
 
 static tree
 cp_parser_init_declarator (cp_parser* parser,
@@ -14328,7 +14352,8 @@  cp_parser_init_declarator (cp_parser* parser,
 			   bool function_definition_allowed_p,
 			   bool member_p,
 			   int declares_class_or_enum,
-			   bool* function_definition_p)
+			   bool* function_definition_p,
+			   tree* just_one_declarator)
 {
   cp_token *token = NULL, *asm_spec_start_token = NULL,
            *attributes_start_token = NULL;
@@ -14492,6 +14517,8 @@  cp_parser_init_declarator (cp_parser* parser,
     {
       is_initialized = SD_INITIALIZED;
       initialization_kind = token->type;
+      if (just_one_declarator)
+	*just_one_declarator = error_mark_node;
 
       if (token->type == CPP_EQ
 	  && function_declarator_p (declarator))
@@ -14505,16 +14532,23 @@  cp_parser_init_declarator (cp_parser* parser,
     }
   else
     {
+      is_initialized = SD_UNINITIALIZED;
+      initialization_kind = CPP_EOF;
       /* If the init-declarator isn't initialized and isn't followed by a
 	 `,' or `;', it's not a valid init-declarator.  */
       if (token->type != CPP_COMMA
 	  && token->type != CPP_SEMICOLON)
 	{
-	  cp_parser_error (parser, "expected initializer");
-	  return error_mark_node;
+	  if (just_one_declarator && *just_one_declarator != error_mark_node)
+	    {
+	      is_initialized = SD_INITIALIZED;
+	    }
+	  else
+	    {
+	      cp_parser_error (parser, "expected initializer");
+	      return error_mark_node;
+	    }
 	}
-      is_initialized = SD_UNINITIALIZED;
-      initialization_kind = CPP_EOF;
     }
 
   /* Because start_decl has side-effects, we should only call it if we
@@ -14590,7 +14624,7 @@  cp_parser_init_declarator (cp_parser* parser,
   initializer = NULL_TREE;
   is_direct_init = false;
   is_non_constant_init = true;
-  if (is_initialized)
+  if (is_initialized && initialization_kind != CPP_EOF)
     {
       if (function_declarator_p (declarator))
 	{
@@ -14658,7 +14692,8 @@  cp_parser_init_declarator (cp_parser* parser,
 
   /* Finish processing the declaration.  But, skip friend
      declarations.  */
-  if (!friend_p && decl && decl != error_mark_node)
+  if (!friend_p && decl && decl != error_mark_node
+      && (!is_initialized || initialization_kind != CPP_EOF))
     {
       cp_finish_decl (decl,
 		      initializer, !is_non_constant_init,
@@ -19904,7 +19939,8 @@  cp_parser_single_declaration (cp_parser* parser,
 				        /*function_definition_allowed_p=*/true,
 				        member_p,
 				        declares_class_or_enum,
-				        &function_definition_p);
+				        &function_definition_p,
+					NULL);
 
     /* 7.1.1-1 [dcl.stc]
 
diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
index 3e8b62c..fc44d46 100644
--- a/gcc/cp/pt.c
+++ b/gcc/cp/pt.c
@@ -12051,7 +12051,7 @@  tsubst_expr (tree t, tree args, tsubst_flags_t complain, tree in_decl,
       }
 
     case FOR_STMT:
-      stmt = begin_for_stmt ();
+      stmt = begin_for_stmt (NULL_TREE, NULL_TREE);
       RECUR (FOR_INIT_STMT (t));
       finish_for_init_stmt (stmt);
       tmp = RECUR (FOR_COND (t));
@@ -12065,7 +12065,7 @@  tsubst_expr (tree t, tree args, tsubst_flags_t complain, tree in_decl,
     case RANGE_FOR_STMT:
       {
         tree decl, expr;
-        stmt = begin_for_stmt ();
+        stmt = begin_for_stmt (NULL_TREE, NULL_TREE);
         decl = RANGE_FOR_DECL (t);
         decl = tsubst (decl, args, complain, in_decl);
         maybe_push_decl (decl);
diff --git a/gcc/cp/semantics.c b/gcc/cp/semantics.c
index 38e03f6..e749b97 100644
--- a/gcc/cp/semantics.c
+++ b/gcc/cp/semantics.c
@@ -826,21 +826,44 @@  finish_return_stmt (tree expr)
   return r;
 }
 
-/* Begin a for-statement.  Returns a new FOR_STMT if appropriate.  */
+/* Begin the scope of a for-statement or a range-for-statement.
+   Both the returned trees are to be used in a call to
+   begin_for_stmt or begin_range_for_stmt.  */
 
 tree
-begin_for_stmt (void)
+begin_for_scope (tree *init)
+{
+  tree scope = NULL_TREE;
+  if (flag_new_for_scope > 0)
+    scope = do_pushlevel (sk_for);
+
+  if (processing_template_decl)
+    *init = push_stmt_list ();
+  else
+    *init = NULL_TREE;
+
+  return scope;
+}
+
+/* Begin a for-statement.  Returns a new FOR_STMT.
+   SCOPE and INIT should be the return of begin_for_scope,
+   or both NULL_TREE  */
+
+tree
+begin_for_stmt (tree scope, tree init)
 {
   tree r;
 
   r = build_stmt (input_location, FOR_STMT, NULL_TREE, NULL_TREE,
 		  NULL_TREE, NULL_TREE);
 
-  if (flag_new_for_scope > 0)
-    TREE_CHAIN (r) = do_pushlevel (sk_for);
-
-  if (processing_template_decl)
-    FOR_INIT_STMT (r) = push_stmt_list ();
+  if (scope == NULL_TREE)
+    {
+      gcc_assert (!init);
+      scope = begin_for_scope (&init);
+    }
+  FOR_INIT_STMT (r) = init;
+  TREE_CHAIN (r) = scope;
 
   return r;
 }
@@ -924,18 +947,29 @@  finish_for_stmt (tree for_stmt)
 }
 
 /* Begin a range-for-statement.  Returns a new RANGE_FOR_STMT.
+   SCOPE and INIT should be the return of begin_for_scope,
+   or both NULL_TREE  .
    To finish it call finish_for_stmt(). */
 
 tree
-begin_range_for_stmt (void)
+begin_range_for_stmt (tree scope, tree init)
 {
   tree r;
 
   r = build_stmt (input_location, RANGE_FOR_STMT,
 		  NULL_TREE, NULL_TREE, NULL_TREE);
 
-  if (flag_new_for_scope > 0)
-    TREE_CHAIN (r) = do_pushlevel (sk_for);
+  if (scope == NULL_TREE)
+    {
+      gcc_assert (!init);
+      scope = begin_for_scope (&init);
+    }
+
+  /* RANGE_FOR_STMTs do not use nor save the init tree, so we
+     pop it now.  */
+  if (init)
+    pop_stmt_list (init);
+  TREE_CHAIN (r) = scope;
 
   return r;
 }
diff --git a/gcc/testsuite/g++.dg/cpp0x/range-for4.C b/gcc/testsuite/g++.dg/cpp0x/range-for4.C
index 96c0d90..afbcf14 100644
--- a/gcc/testsuite/g++.dg/cpp0x/range-for4.C
+++ b/gcc/testsuite/g++.dg/cpp0x/range-for4.C
@@ -3,8 +3,6 @@ 
 // { dg-do run }
 // { dg-options "-std=c++0x" }
 
-#include <cstdio>
-
 /* Preliminary declarations */
 namespace pre
 {
@@ -48,7 +46,6 @@  container run_me_just_once()
 }
 
 /* Template with dependent expression. */
-/* Template with dependent expression. */
 template<typename T> int test1(const T &r)
 {
   int t = 0;