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