On November 10, 2016 6:38:12 PM GMT+01:00, Joseph Myers
<jos...@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.
