[clang-tools-extra] 74f859b - [clang-tidy] Fix readability-trailing-comma false positive on #endif (#219634)

[via cfe-commits]

Author: Aayush Mainali
Date: 2026-09-28T13:02:08Z
New Revision: 74f859b9d46e480fe43d7b904b1e80efb8a331a2

URL: https://github.com/llvm/llvm-project/commit/74f859b9d46e480fe43d7b904b1e80efb8a331a2
DIFF: https://github.com/llvm/llvm-project/commit/74f859b9d46e480fe43d7b904b1e80efb8a331a2.diff

LOG: [clang-tidy] Fix readability-trailing-comma false positive on #endif (#219634)

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

Added: 
    

Modified: 
    clang-tools-extra/clang-tidy/readability/TrailingCommaCheck.cpp
    clang-tools-extra/docs/ReleaseNotes.md
    clang-tools-extra/test/clang-tidy/checkers/readability/trailing-comma-cxx11.cpp
    clang-tools-extra/test/clang-tidy/checkers/readability/trailing-comma.cpp

Removed: 
    


################################################################################
diff  --git a/clang-tools-extra/clang-tidy/readability/TrailingCommaCheck.cpp b/clang-tools-extra/clang-tidy/readability/TrailingCommaCheck.cpp
index d687980ed0999..c0ed9849304de 100644
--- a/clang-tools-extra/clang-tidy/readability/TrailingCommaCheck.cpp
+++ b/clang-tools-extra/clang-tidy/readability/TrailingCommaCheck.cpp
@@ -44,6 +44,18 @@ static bool isSingleLine(SourceRange Range, const SourceManager &SM) {
          SM.getExpansionLineNumber(Range.getEnd());
 }
 
+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) &&
+         SM.getExpansionLineNumber(Prev->getLocation()) ==
+             SM.getExpansionLineNumber(Tok.getLocation());
+}
+
 namespace {
 
 AST_POLYMORPHIC_MATCHER(isMacro,
@@ -110,12 +122,16 @@ 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;
 
+  if (isPreprocessorDirectiveToken(*LastTok, *Result.SourceManager,
+                                   getLangOpts()))
+    return;
+
   emitDiag(LastTok->getLocation(), LastTok, DiagKind::Enum, Result, Policy);
 }
 

diff  --git a/clang-tools-extra/docs/ReleaseNotes.md b/clang-tools-extra/docs/ReleaseNotes.md
index 3d9e34cc4d44e..5ad0b2d3718b9 100644
--- a/clang-tools-extra/docs/ReleaseNotes.md
+++ b/clang-tools-extra/docs/ReleaseNotes.md
@@ -326,6 +326,10 @@ infrastructure are described first, followed by tool-specific sections.
     synthesized for intermediate subobjects caused the trailing comma of the
     enclosing list to be incorrectly rewritten.
 
+  - Ignored preprocessor directives such as `#endif` that appear immediately
+    before an enum's closing brace, which previously produced a false positive
+    and a fix-it that inserted a comma after the directive.
+    
   - Fixed a false positive on empty brace initializers of types with default
     member initializers.
 

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 5d836b7727082..79cce3c5e68cf 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
@@ -55,6 +55,19 @@ struct PackSingle {
 PackSingle<int> p1;
 PackSingle<int, double, char> p3;
 
+// #endif before '}' is not a missing trailing comma.
+enum class color_t : unsigned {
+  RED = 0,
+  GREEN = 1,
+  BLUE = 2,
+  CYAN = 3,
+#ifdef USE_MAGENTA
+  LAST = CYAN,
+#else
+  LAST = BLUE,
+#endif
+};
+
 struct WithDefault { int foo = 1; };
 void takesTwo(WithDefault, int);
 

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..fb15931731b07 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,52 @@ void nestedMultiLine() {
   // CHECK-FIXES-NEXT:   };
 }
 
+// #endif before '}' is not a missing trailing comma.
+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
+};
+
+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: };
+
+enum NullDirectiveBefore {
+#
+  ND_A
+};
+// CHECK-MESSAGES: :[[@LINE-2]]:7: warning: enum should have a trailing comma
+// CHECK-FIXES: enum NullDirectiveBefore {
+// CHECK-FIXES-NEXT: #
+// CHECK-FIXES-NEXT:   ND_A,
+// CHECK-FIXES-NEXT: };
+
+enum NullDirectiveAfter {
+  ND_B,
+#
+};
+
 // Macros are ignored
 #define ENUM(n, a, b) enum n { a, b }
 #define INIT {1, 2}