On Tue, 2018-12-11 at 10:35 -0500, David Malcolm wrote:
> On Mon, 2018-12-10 at 22:47 +0000, Segher Boessenkool wrote:
>
> [...]
>
> > diff --git a/gcc/c/c-parser.c b/gcc/c/c-parser.c
> > index 121a91c..652e53c 100644
> > --- a/gcc/c/c-parser.c
> > +++ b/gcc/c/c-parser.c
> > @@ -6360,41 +6360,54 @@ c_parser_for_statement (c_parser *parser,
> > bool ivdep, unsigned short unroll,
> > static tree
> > c_parser_asm_statement (c_parser *parser)
> > {
> > - tree quals, str, outputs, inputs, clobbers, labels, ret;
> > - bool simple, is_volatile, is_inline, is_goto;
> > + tree str, outputs, inputs, clobbers, labels, ret;
> > + bool simple;
> > location_t asm_loc = c_parser_peek_token (parser)->location;
> > int section, nsections;
> >
> > gcc_assert (c_parser_next_token_is_keyword (parser, RID_ASM));
> > c_parser_consume_token (parser);
> >
> > - quals = NULL_TREE;
> > - is_volatile = false;
> > - is_inline = false;
> > - is_goto = false;
> > + /* Handle the asm-qualifier-list. */
> > + location_t volatile_loc = UNKNOWN_LOCATION;
> > + location_t inline_loc = UNKNOWN_LOCATION;
> > + location_t goto_loc = UNKNOWN_LOCATION;
> > for (;;)
> > {
> > - switch (c_parser_peek_token (parser)->keyword)
> > + c_token *token = c_parser_peek_token (parser);
> > + location_t loc = token->location;
> > + switch (token->keyword)
> > {
> > case RID_VOLATILE:
> > - if (is_volatile)
> > - break;
> > - is_volatile = true;
> > - quals = c_parser_peek_token (parser)->value;
> > + if (volatile_loc)
> > + {
> > + error_at (loc, "duplicate asm qualifier %qE", token-
> > > value);
> >
> > + inform (volatile_loc, "first seen here");
> > + }
>
> Thanks for the improvements.
>
> Is there test coverage for these errors and notes?
>
> A diagnostic nit (new with gcc 9): please add an:
> auto_diagnostic_group d;
> to the start of the guarded block, so that the "error" and "note" are
> known to be related.
>
> See:
> https://gcc.gnu.org/onlinedocs/gccint/Guidelines-for-Diagnostics.html
> #Group-logically-related-diagnostics
For bonus points, these could offer fix-it hints, so that an IDE can
offer to delete the duplicate qualifier token. The above code could
be:
if (volatile_loc)
complain_about_duplicate_asm_qualifier (token->value, loc,
volatile_loc);
else
volatile_loc = loc;
void
complain_about_duplicate_asm_qualifier (tree value,
location_t duplicate_loc,
location_t first_loc)
{
auto_diagnostic_group d;
gcc_rich_location richloc (duplicate_loc);
richloc.add_fixit_remove ();
error_at (&richloc, "duplicate asm qualifier %qE", value);
inform (first_loc, "first seen here");
}
or somesuch, where rich_location::add_fixit_remove adds a fix-it hint
suggesting the removal of all of "loc", the duplicate token; given that
it's 5 lines at this point, a subroutine seems justified, to eliminate
duplication at the 6 sites it's done.
Caveat: haven't tried to compile the above.
Dave
> > + else
> > + volatile_loc = loc;
> > c_parser_consume_token (parser);
> > continue;
> >
> > case RID_INLINE:
> > - if (is_inline)
> > - break;
> > - is_inline = true;
> > + if (inline_loc)
> > + {
> > + error_at (loc, "duplicate asm qualifier %qE", token-
> > > value);
> >
> > + inform (inline_loc, "first seen here");
>
> Likewise.
>
> > + }
> > + else
> > + inline_loc = loc;
> > c_parser_consume_token (parser);
> > continue;
> >
> > case RID_GOTO:
> > - if (is_goto)
> > - break;
> > - is_goto = true;
> > + if (goto_loc)
> > + {
> > + error_at (loc, "duplicate asm qualifier %qE", token-
> > > value);
> >
> > + inform (goto_loc, "first seen here");
> > + }
>
> Likewise.
>
> > + else
> > + goto_loc = loc;
> > c_parser_consume_token (parser);
> > continue;
>
> [...]
>
> > diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
> > index 1cc34ba..06a6bb0 100644
> > --- a/gcc/cp/parser.c
> > +++ b/gcc/cp/parser.c
> > @@ -19649,29 +19646,50 @@ cp_parser_asm_definition (cp_parser*
> > parser)
> > }
> >
> > /* Handle the asm-qualifier-list. */
> > + location_t volatile_loc = UNKNOWN_LOCATION;
> > + location_t inline_loc = UNKNOWN_LOCATION;
> > + location_t goto_loc = UNKNOWN_LOCATION;
> > if (cp_parser_allow_gnu_extensions_p (parser))
> > for (;;)
> > {
> > + cp_token *token = cp_lexer_peek_token (parser->lexer);
> > + location_t loc = token->location;
> > switch (cp_lexer_peek_token (parser->lexer)->keyword)
> > {
> > case RID_VOLATILE:
> > - if (volatile_p)
> > - break;
> > - volatile_p = true;
> > + if (volatile_loc)
> > + {
> > + error_at (loc, "duplicate asm qualifier %qT",
> > token-
> > > u.value);
> >
> > + inform (volatile_loc, "first seen here");
>
> Likewise.
>
> > + }
> > + else
> > + volatile_loc = loc;
> > cp_lexer_consume_token (parser->lexer);
> > continue;
> >
> > case RID_INLINE:
> > - if (inline_p || !parser->in_function_body)
> > + if (!parser->in_function_body)
> > break;
> > - inline_p = true;
> > + if (inline_loc)
> > + {
> > + error_at (loc, "duplicate asm qualifier %qT",
> > token-
> > > u.value);
> >
> > + inform (inline_loc, "first seen here");
>
> Likewise.
>
> > + }
> > + else
> > + inline_loc = loc;
> > cp_lexer_consume_token (parser->lexer);
> > continue;
> >
> > case RID_GOTO:
> > - if (goto_p || !parser->in_function_body)
> > + if (!parser->in_function_body)
> > break;
> > - goto_p = true;
> > + if (goto_loc)
> > + {
> > + error_at (loc, "duplicate asm qualifier %qT",
> > token-
> > > u.value);
> >
> > + inform (goto_loc, "first seen here");
>
> Likewise.
>
> > + }
> > + else
> > + goto_loc = loc;
> > cp_lexer_consume_token (parser->lexer);
> > continue;
> >
>
> [...]
>
>
> Dave
