aaron.ballman added inline comments.
================
Comment at: clang/lib/Parse/ParseStmtAsm.cpp:704
+ AQ = DeclSpec::AQ_goto;
+ else {
+ if (EndLoc.isValid())
----------------
nickdesaulniers wrote:
> aaron.ballman wrote:
> > I would expect a diagnostic if the unknown token is anything other than a
> > left paren.
> That currently is is handled by the caller, see `if (Tok.isNot(tok::l_paren))
> {` in `Parser::ParseAsmStatement` (line 762 of
> clang/lib/Parse/ParseStmtAsm.cpp).
My suggestion is to move it out of the caller and into the helper. This
function is expected to parse the asm qualifiers, so it stands to reason if it
gets something other than an asm qualifier, it should diagnose. (We probably
won't need to re-use this parsing logic in other contexts, but that's why it's
better to have the diagnostic here rather than assume the caller will do it.)
================
Comment at: clang/lib/Parse/ParseStmtAsm.cpp:725
/// [GNU] gnu-asm-statement:
-/// 'asm' type-qualifier[opt] '(' asm-argument ')' ';'
+/// 'asm' asm-qualifier[opt] '(' asm-argument ')' ';'
///
----------------
nickdesaulniers wrote:
> I guess this should be `asm-qualifier-list[opt]`?
Yup, good catch!
================
Comment at: clang/test/Parser/asm-qualifiers.c:12-14
+ asm noodle(""); // expected-error {{expected '(' after 'asm'}}
+ asm goto noodle("" ::::foo); // expected-error {{expected '(' after 'asm'}}
+ asm volatile noodle inline(""); // expected-error {{expected '(' after
'asm'}}
----------------
I think these diagnostics should be improved as none of them suggest the
underlying problem. As they read now, it sounds like you can write
`asm(volatile noodle inline(""))` to appease the diagnostic.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D75563/new/
https://reviews.llvm.org/D75563
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
