Re: PING: Fwd: Re: [patch] implement Cilk Plus simd loops on trunk

[Aldy Hernandez <aldyh at redhat dot com>]

On 10/02/13 16:53, Jason Merrill wrote:

Sorry for the delay, and thank you for the review.

> On 08/27/2013 04:03 PM, Aldy Hernandez wrote:
> > +  /* First, try to parse as an initialized declaration.  See
> > +     cp_parser_condition, from whence the bulk of this is copied.  */
>
> You didn't do this copy, but I'd appreciate it if you could reintegrate
> this with cp_parser_condition.  I notice that there's already been
> significant drift: this copied code doesn't recognize {} initialization.

This has the potential of throwing my mind for a spin.  Can I do this as 
a followup, and keep it simple for now?

> > +          else if (!TREE_TYPE (e) || !TREE_CONSTANT (e)
> > +               || !INTEGRAL_TYPE_P (TREE_TYPE (e)))
> > +        cp_parser_error (parser,
> > +                 "step size must be an integer constant");
>
> Can't the step size be a value-dependent expression like a template
> non-type parameter?

Fixed.  I also found some nuances in how we were (NOT) handling 
variables as the standard mentioned, both for C and C++.  Fixed as well, 
and added testcases when appropriate.

> > +  if (cp_lexer_next_token_is_keyword (parser->lexer, RID_STATIC)
> > +      || cp_lexer_next_token_is_keyword (parser->lexer, RID_REGISTER)
> > +      || cp_lexer_next_token_is_keyword (parser->lexer, RID_EXTERN)
> > +      || cp_lexer_next_token_is_keyword (parser->lexer, RID_MUTABLE)
> > +      || cp_lexer_next_token_is_keyword (parser->lexer, RID_THREAD))
> > +    {
> > +      error_at (loc, "storage class is not allowed");
> > +      cp_lexer_consume_token (parser->lexer);
> > +    }
>
> How is this different from any other for-loop?

Sigh, frustratingly because the standard says so.

> > +  gcc_assert (for_keyword == RID_FOR);
> > +
> > +  if (!cp_lexer_next_token_is_keyword (parser->lexer, for_keyword))
> > +    {
> > +      if (for_keyword == RID_FOR)
>
> A lot of code in this function tests for_keyword after the assert.  A
> comment lower down says
>
> > +      /* Handle _Cilk_for here when implemented.  */
> > +      gcc_unreachable ();
>
> So do we need the earlier assert?  At least add a comment to it.

Removed.

How does this look?

Aldy

Attachment: curr
Description: Text document