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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
