llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-tools-extra Author: Aayush Mainali (AayushMainali-Github) <details> <summary>Changes</summary> The `readability-trailing-comma` check looks at the token immediately before an enum's closing brace to decide whether a trailing comma is present. When the last enumerators are wrapped in `#ifdef` / `#else` / `#endif`, that token is the directive identifier `endif`, so the check reports a missing comma and `-fix` inserts `,` after `#endif`. That is outside the enumerator list and corrupts the source even when every enumerator already has a trailing comma. Skip the diagnostic when the token before `}` is a preprocessor directive (`#` or a token preceded by `#`). An enumerator whose name happens to be `endif` is still diagnosed, because it is not preceded by `#`. Fixes #<!-- -->218957 --- Full diff: https://github.com/llvm/llvm-project/pull/219634.diff 3 Files Affected: - (modified) clang-tools-extra/clang-tidy/readability/TrailingCommaCheck.cpp (+25-3) - (modified) clang-tools-extra/test/clang-tidy/checkers/readability/trailing-comma-cxx11.cpp (+14) - (modified) clang-tools-extra/test/clang-tidy/checkers/readability/trailing-comma.cpp (+34) ``````````diff diff --git a/clang-tools-extra/clang-tidy/readability/TrailingCommaCheck.cpp b/clang-tools-extra/clang-tidy/readability/TrailingCommaCheck.cpp index cb1a33ba09233..60fa53ddf6556 100644 --- a/clang-tools-extra/clang-tidy/readability/TrailingCommaCheck.cpp +++ b/clang-tools-extra/clang-tidy/readability/TrailingCommaCheck.cpp @@ -56,6 +56,19 @@ AST_MATCHER(EnumDecl, isEmptyEnum) { return Node.enumerators().empty(); } AST_MATCHER(InitListExpr, isEmptyInitList) { return Node.getNumInits() == 0; } +// True when Tok is a preprocessor directive (the '#' or the directive +// identifier such as 'endif'). Those tokens can sit between the last +// enumerator and '}', and must not be treated as a missing trailing comma. +static bool isPreprocessorDirectiveToken(const Token &Tok, + const SourceManager &SM, + const LangOptions &LangOpts) { + if (Tok.is(tok::hash)) + return true; + const std::optional<Token> Prev = Lexer::findPreviousToken( + Tok.getLocation(), SM, LangOpts, /*IncludeComments=*/false); + return Prev && Prev->is(tok::hash); +} + } // namespace TrailingCommaCheck::TrailingCommaCheck(StringRef Name, @@ -110,12 +123,21 @@ void TrailingCommaCheck::checkEnumDecl(const EnumDecl *Enum, if (Policy == CommaPolicyKind::Ignore) return; - const std::optional<Token> LastTok = - Lexer::findPreviousToken(Enum->getBraceRange().getEnd(), - *Result.SourceManager, getLangOpts(), false); + const std::optional<Token> LastTok = Lexer::findPreviousToken( + Enum->getBraceRange().getEnd(), *Result.SourceManager, getLangOpts(), + /*IncludeComments=*/false); if (!LastTok) return; + // `#endif` (and similar directives) can appear immediately before the + // closing brace when enumerators are guarded by `#ifdef`. Walking back from + // `}` would otherwise treat that directive as the last enumerator and insert + // a comma after it, even when every active enumerator already has a trailing + // comma. + if (isPreprocessorDirectiveToken(*LastTok, *Result.SourceManager, + getLangOpts())) + return; + emitDiag(LastTok->getLocation(), LastTok, DiagKind::Enum, Result, Policy); } diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/trailing-comma-cxx11.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability/trailing-comma-cxx11.cpp index 9f37db2c837c3..80c88fcc2396c 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/readability/trailing-comma-cxx11.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/readability/trailing-comma-cxx11.cpp @@ -54,3 +54,17 @@ struct PackSingle { PackSingle<int> p1; PackSingle<int, double, char> p3; + +// Preprocessor-guarded enumerators already have trailing commas; do not insert +// a comma after '#endif'. +enum class color_t : unsigned { + RED = 0, + GREEN = 1, + BLUE = 2, + CYAN = 3, +#ifdef USE_MAGENTA + LAST = CYAN, +#else + LAST = BLUE, +#endif +}; diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/trailing-comma.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability/trailing-comma.cpp index 76fb4bbf0c37d..daef89b770716 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/readability/trailing-comma.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/readability/trailing-comma.cpp @@ -144,6 +144,40 @@ void nestedMultiLine() { // CHECK-FIXES-NEXT: }; } +// Preprocessor directives immediately before '}' must not be treated as the +// last enumerator. Both branches already have trailing commas; a false +// positive would insert a comma after '#endif'. +enum color_t { + COLOR_RED = 0, + COLOR_GREEN = 1, + COLOR_BLUE = 2, + COLOR_CYAN = 3, +#ifdef USE_MAGENTA + COLOR_LAST = COLOR_CYAN, +#else + COLOR_LAST = COLOR_BLUE, +#endif +}; + +enum GuardedEnumerator { + GE_A, + GE_B, +#ifdef USE_EXTRA + GE_C, +#endif +}; + +// An enumerator named 'endif' is still diagnosed; only '#endif' is ignored. +enum EndsWithEndifName { + foo, + endif +}; +// CHECK-MESSAGES: :[[@LINE-2]]:8: warning: enum should have a trailing comma +// CHECK-FIXES: enum EndsWithEndifName { +// CHECK-FIXES-NEXT: foo, +// CHECK-FIXES-NEXT: endif, +// CHECK-FIXES-NEXT: }; + // Macros are ignored #define ENUM(n, a, b) enum n { a, b } #define INIT {1, 2} `````````` </details> https://github.com/llvm/llvm-project/pull/219634 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
