Re: [PATCH 1/2] asm qualifiers (PR55681)

[Segher Boessenkool <segher@kernel.crashing.org>]

Hi!

On Wed, Dec 05, 2018 at 04:47:37PM -0500, Jason Merrill wrote:
> On 12/2/18 11:38 AM, Segher Boessenkool wrote:
> >PR55681 observes that currently only one qualifier is allowed for
> >inline asm, so that e.g. "volatile asm" is allowed, "const asm" is also
> >okay (with a warning), but "const volatile asm" gives an error.  Also
> >"goto" has to be last.
> >
> >This patch changes things so that only "asm-qualifiers" are allowed,
> >that is "volatile" and "goto", in any combination, in any order, but
> >without repetitions.
> >
> >
> >2018-12-02  Segher Boessenkool  <segher@kernel.crashing.org>
> >
> >	PR inline-asm/55681
> >	* doc/extend.texi (Basic Asm): Update grammar.
> >	(Extended Asm): Update grammar.
> >
> >gcc/c/
> >	PR inline-asm/55681
> >	* c-parser.c (c_parser_for_statement): Update grammar.  Allow any
> >	combination of volatile and goto, in any order, without repetitions.
> >
> >gcc/cp/
> >	PR inline-asm/55681
> >	* parser.c (cp_parser_using_directive): Update grammar.  Allow any
> >	combination of volatile and goto, in any order, without repetitions.
> 
> You don't actually change cp_parser_using_directive, despite what diff 
> says: you're changing cp_parser_asm_definition.

I trust diff too much, sigh.

> >+    for (bool done = false; !done ; )
> >+      switch (cp_lexer_peek_token (parser->lexer)->keyword)
> >+	{
> >+	case RID_VOLATILE:
> >+	  if (!volatile_p)
> >+	    {
> >+	      /* Remember that we saw the `volatile' keyword.  */
> >+	      volatile_p = true;
> >+	      /* Consume the token.  */
> >+	      cp_lexer_consume_token (parser->lexer);
> >+	    }
> >+	  else
> >+	    done = true;
> >+	  break;
> >+	case RID_GOTO:
> >+	  if (!goto_p && parser->in_function_body)
> >+	    {
> >+	      /* Remember that we saw the `goto' keyword.  */
> >+	      goto_p = true;
> >+	      /* Consume the token.  */
> >+	      cp_lexer_consume_token (parser->lexer);
> >+	    }
> >+	  else
> >+	    done = true;
> >+	  break;
> >+	default:
> >+	  done = true;
> >+	}
> 
> An arguably simpler alternative to using the 'done' variable would be to 
> 'break' out of the loop after the switch, and have the consume_token 
> cases explicitly 'continue'.

Yeah, that is neater, continue only deals with the loop.  Nice.

> We also might remember the old tokens and give a more helpful error 
> message in the case of duplicate keywords.
> 
> But I won't insist on either of these, the C++ changes are OK as-is.

I'll commit it like this then, and work on the improvements afterwards
(they also apply to the C frontend).

Thanks for the review!


Segher