[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/include/clang/Basic/DiagnosticParseKinds.td:32
   "'asm goto' cannot have output constraints">;
+def err_asm_duplicate_qual : Error<"duplicate asm qualifier %0">;
 }
----------------
`duplicate asm qualifier '%0'` (with the quotes around the `%0`).


================
Comment at: clang/include/clang/Sema/DeclSpec.h:324
 
+  // GNU Asm Qualifiers
+  enum AQ {
----------------
Asm Qualifiers -> asm qualifiers


================
Comment at: clang/include/clang/Sema/DeclSpec.h:378
+  // GNU asm specifier
+  unsigned GNUAsmQualifiers : 4;
+
----------------
I think we only need three bits to store this. Also, the comment should be clear that this is a bitwise OR of AQ, similar to the comment for `TypeQualifiers`.


================
Comment at: clang/include/clang/Sema/DeclSpec.h:582
+  /// getGNUAsmQualifiers - Return a set of AQs.
+  unsigned GetGNUAsmQual() const { return GNUAsmQualifiers; }
+
----------------
The comment and the code don't match. I prefer the spelling in the comment.


================
Comment at: clang/include/clang/Sema/DeclSpec.h:736
 
+  bool SetGNUAsmQual(AQ A, SourceLocation Loc);
+
----------------
Given that we're already inconsistent in this area regarding naming, I'd prefer to follow the LLVM naming convention and use `setGNUAsmQualifier()`


================
Comment at: clang/lib/Parse/ParseStmtAsm.cpp:688
+/// ParseGNUAsmQualifierListOpt - Parse a GNU extended asm qualifier list.
+///       asm-qualifiers:
+///         volatile
----------------
The comment doesn't match the code -- I think the comment should be:
```
/// asm-qualifier:
///   volatile
///   inline
///   goto
///
/// asm-qualifier-list:
///   asm-qualifier
///   asm-qualifier-list asm-qualifier
```


================
Comment at: clang/lib/Parse/ParseStmtAsm.cpp:704
+      AQ = DeclSpec::AQ_goto;
+    else {
+      if (EndLoc.isValid())
----------------
I would expect a diagnostic if the unknown token is anything other than a left paren.


================
Comment at: clang/test/Parser/asm-qualifiers.c:3
+
+void qualifiers(void) {
+  asm("");
----------------
There should also be tests where the qualifiers are incorrect. e.g.,
```
asm noodle("");
asm goto noodle("");
asm volatile noodle inline("");
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75563/new/

https://reviews.llvm.org/D75563