[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:17
+  "expected 'volatile', 'inline', 'goto', or '('">, CatInlineAsm;
+def err_global_asm_qualifier_ignored : Error<
+  "meaningless '%0' on asm outside function">, CatInlineAsm;
----------------
If we know of any compilers that support asm statements with qualifiers other than what Clang supports, it may make sense for these diagnostics to be warnings that are treated as an error by default so that users can disable the warning when compiling with Clang. I don't know of any such compilers off the top of my head, but perhaps you know of some.


================
Comment at: clang/include/clang/Parse/Parser.h:3215
+    GNUAsmQualifiers() : Qualifiers(0) {}
+    static const char *getSpecifierName(const AQ AQ);
+    static AQ getQualifierForTokenKind(const tok::TokenKind K);
----------------
nickdesaulniers wrote:
> Now that we're not using `DeclSpec`, maybe we could drop this in preference of `operator<<`.
I prefer the named function to a streaming operator given how little this will be used -- it's easier to call the named function from within a debugger than to use a streaming operator.

Please don't use a type name as a parameter identifier. Also, drop the top-level const.


================
Comment at: clang/include/clang/Parse/Parser.h:3206
+  class GNUAsmQualifiers {
+    unsigned Qualifiers : 3; // Bitwise OR of AQ.
+  public:
----------------
There's really no benefit to using a bit-field here, so I'd just store an `unsigned`.


================
Comment at: clang/include/clang/Parse/Parser.h:3214
+    };
+    GNUAsmQualifiers() : Qualifiers(0) {}
+    static const char *getSpecifierName(const AQ AQ);
----------------
I think you should use an in-class initializer for `Qualifiers` and then can drop the constructor entirely.


================
Comment at: clang/include/clang/Parse/Parser.h:3216
+    static const char *getSpecifierName(const AQ AQ);
+    bool setAsmQualifier(const AQ AQ);
+    inline bool isVolatile() const { return Qualifiers & AQ_volatile; };
----------------
Please don't use a type name as a parameter identifier. Also, drop the top-level `const`.


================
Comment at: clang/include/clang/Parse/Parser.h:3224
+  GNUAsmQualifiers::AQ getGNUAsmQualifier(const Token &Tok) const;
+  bool ParseGNUAsmQualifierListOpt(GNUAsmQualifiers *AQ);
 };
----------------
This should be taking a reference rather than a pointer. It should also be named `parseGNUAsmQualifierListOpt()` with a lowercase `p`.


================
Comment at: clang/lib/Parse/ParseStmtAsm.cpp:358
+bool Parser::isGNUAsmQualifier(const Token &TokAfterAsm) const {
+  return !!getGNUAsmQualifier(TokAfterAsm);
 }
----------------
This is too clever for my tastes -- I'd prefer an explicit test against `AQ_unspecified`.


================
Comment at: clang/lib/Parse/ParseStmtAsm.cpp:932
+
+const char *Parser::GNUAsmQualifiers::getSpecifierName(const AQ AQ) {
+  switch (AQ) {
----------------
type vs param name and top-level `const`.


================
Comment at: clang/lib/Parse/ParseStmtAsm.cpp:937
+    case AQ_goto: return "goto";
+    case AQ_unspecified:;
+  }
----------------
This looks wrong to me -- it flows through to an unreachable despite being reachable. I think this should `return "unspecified";`.


================
Comment at: clang/lib/Parse/ParseStmtAsm.cpp:951
+}
+bool Parser::GNUAsmQualifiers::setAsmQualifier(const AQ AQ) {
+  bool IsDuplicate = Qualifiers & AQ;
----------------
type vs param name and top-level `const`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75563