On 10/23/2015 01:31 PM, Jakub Jelinek wrote:
> 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
> {
> ...
> }
> }
> ?
My plan over here was try and catch any arguments with a colon. But that
fell threw because...
>> + 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).
I'm not handling commas properly. My next patch is going to handle the
static argument separately.
>> + /* 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.
I'll include a test case for this with the next patch.
Cesar
