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