[C++0x patch] implement range-based for loops

Jason Merrill jason@redhat.com
Mon Aug 16 22:07:00 GMT 2010


Since it sounds like your assignment is coming along, I'll go ahead and 
review this now.  It looks good, but there are a few issues:

On 07/23/2010 08:13 AM, Rodrigo Rivas wrote:
> +    tree max = TYPE_MAX_VALUE (TYPE_DOMAIN (TREE_TYPE (range_expr)));
> +    end_expr = build_binary_op (UNKNOWN_LOCATION, PLUS_EXPR, range_expr,
> +               build_binary_op (UNKNOWN_LOCATION, PLUS_EXPR, max, build_one_cst (integer_type_node), 0), 0);

It would be shorter to array_type_nelts_top (TREE_TYPE (range_expr)) here.

Also, a number of the lines in your patch go past 80 columns; please 
reformat so they don't.  And when wrapping function arguments to the 
next line, they should start at the column after the '(' of the call.

> +    iter_type = TREE_TYPE (begin_expr);

I believe this will get the wrong type if the begin/end functions have a 
return type of const Class; the iterator should have the cv-unqualified 
version.

> +  if (pushed_scope) /* No scopes allowed here */
> +      pop_scope(pushed_scope);

You don't need to test for NULL before calling pop_scope anymore; I 
fixed it to accept a NULL argument recently.

Also, the second line here is indented too far, and you need a space 
before the '('.

I'm guessing this doesn't work with templates yet?  You don't have any 
template testcases.

Thanks for the contribution!

Jason



More information about the Gcc-patches mailing list