On Fri, Oct 23, 2015 at 01:17:07PM -0700, Cesar Philippidis wrote:
> Good idea, thanks. This patch also corrects the problems parsing weird
> combinations of num, static and length arguments that you mentioned
> elsewhere.
>
> Is this OK for trunk?
I'd strongly prefer to see always patches accompanied by testcases.
> + loc = c_parser_peek_token (parser)->location;
> + op_to_parse = &op0;
> +
> + if ((c_parser_next_token_is (parser, CPP_NAME)
> + || c_parser_next_token_is (parser, CPP_KEYWORD))
> + && c_parser_peek_2nd_token (parser)->type == CPP_COLON)
> + {
> + tree name_kind = c_parser_peek_token (parser)->value;
> + const char *p = IDENTIFIER_POINTER (name_kind);
I think I'd prefer not to peek at this at all if it is RID_STATIC,
so perhaps just have (and name_kind is weird):
else
{
tree val = c_parser_peek_token (parser)->value;
if (strcmp (id, IDENTIFIER_POINTER (val)) == 0)
{
c_parser_consume_token (parser); /* id */
c_parser_consume_token (parser); /* ':' */
}
else
{
...
}
}
?
> + if (kind == OMP_CLAUSE_GANG
> + && c_parser_next_token_is_keyword (parser, RID_STATIC))
> + {
> + c_parser_consume_token (parser); /* static */
> + c_parser_consume_token (parser); /* ':' */
> +
> + op_to_parse = &op1;
> + if (c_parser_next_token_is (parser, CPP_MULT))
> + {
> + c_parser_consume_token (parser);
> + *op_to_parse = integer_minus_one_node;
> +
> + /* Consume a comma if present. */
> + if (c_parser_next_token_is (parser, CPP_COMMA))
> + c_parser_consume_token (parser);
Doesn't this mean that you happily parse
gang (static: * abc)
or
gang (static:*num:1)
etc.? I'd say the comma should be non-optional (i.e. either accept
CPP_COMMA, or CPP_CLOSE_PARENT, but nothing else) in that case (at least,
when in OpenMP grammar something is *-list it is meant to be comma
separated).
> + /* Consume a comma if present. */
> + if (c_parser_next_token_is (parser, CPP_COMMA))
> + c_parser_consume_token (parser);
Similarly this means
gang (num: 5 static: *)
is accepted. If it is valid, then again it should have testsuite coverage.
Jakub
