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

[via cfe-commits]

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