Re: Re: [OpenACC 4/11] C FE changes

[Jakub Jelinek <jakub at redhat dot com>]

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