[PATCH][1/2] GIMPLE Frontend, C FE parts (and GIMPLE parser)

[Richard Biener]

On November 10, 2016 6:38:12 PM GMT+01:00, Joseph Myers <joseph@codesourcery.com> wrote:
>On Fri, 28 Oct 2016, Richard Biener wrote:
>
>> +/* Parse a gimple expression.
>> +
>> +   gimple-expression:
>> +     gimple-unary-expression
>> +     gimple-call-statement
>> +     gimple-binary-expression
>> +     gimple-assign-expression
>> +     gimple-cast-expression
>
>I don't see any comments expanding what the syntax is for most of these
>
>constructs.
>
>> +  if (c_parser_next_token_is (parser, CPP_EQ))
>> +    c_parser_consume_token (parser);
>
>That implies you're allowing an optional '=' at this point in the
>syntax.  
>That doesn't seem to make sense to me; I'd expect you to do if (=) { 
>process assignment; } else { other cases; } or similar.
>
>> +  /* GIMPLE PHI expression.  */
>> +  if (c_parser_next_token_is_keyword (parser, RID_PHI))
>
>I don't see this mentioned in any of the syntax comments.
>
>> +  struct {
>> +    /* The expression at this stack level.  */
>> +    struct c_expr expr;
>> +    /* The operation on its left.  */
>> +    enum tree_code op;
>> +    /* The source location of this operation.  */
>> +    location_t loc;
>> +  } stack[2];
>> +  int sp;
>> +  /* Location of the binary operator.  */
>> +  location_t binary_loc = UNKNOWN_LOCATION;  /* Quiet warning.  */
>> +#define POP								      \
>
>This all looks like excess complexity.  The syntax in the comment 
>indicates that in GIMPLE, the operands of a binary expression are unary
>
>expressions.  So nothing related to precedence is needed at all, and
>you 
>shouldn't need this stack construct.

I'll address those comments.  As you did not have any comments on the c-parser.[CH] parts does that mean you are fine with them?  That is, does the above constitute a complete review of the patch?

Thanks,
Richard.