[PATCH] D75563: [clang][Parse] properly parse asm-qualifiers, asm inline

[Aaron Ballman via Phabricator via cfe-commits]

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