[clang-tools-extra] [clang-tidy] Fix false positive in readability-redundant-preprocessor for builtin checks (PR #181734)
Aditya Singh via cfe-commits
cfe-commits at lists.llvm.org
Mon Feb 16 12:00:52 PST 2026
https://github.com/Aditya26189 updated https://github.com/llvm/llvm-project/pull/181734
>From 2714f02bd42389ef227d8a3df1fcd8f7d12d3d80 Mon Sep 17 00:00:00 2001
From: Aditya Singh <139976506+Aditya26189 at users.noreply.github.com>
Date: Tue, 17 Feb 2026 01:17:44 +0530
Subject: [PATCH] [clang-tidy] Fix false positive in
readability-redundant-preprocessor for builtin checks
The ConditionRange parameter passed to If() callback points to incomplete
token locations after macro expansion. For builtin expressions like
__has_builtin(__remove_cvref), the preprocessor's ExpandBuiltinMacro
replaces the entire expression with a single numeric token, causing the
condition range to collapse.
Implemented getConditionText() helper that reads the condition text
directly from the source buffer using Lexer::getLocForEndOfToken() on
the 'if' keyword location, handling backslash line continuations.
Fixes: llvm/llvm-project#64825
---
.../RedundantPreprocessorCheck.cpp | 50 +++++-
.../readability/redundant-preprocessor.cpp | 150 ++++++++++++++++++
2 files changed, 196 insertions(+), 4 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/readability/RedundantPreprocessorCheck.cpp b/clang-tools-extra/clang-tidy/readability/RedundantPreprocessorCheck.cpp
index 4c503714346f8..b31e8d5f4c51a 100644
--- a/clang-tools-extra/clang-tidy/readability/RedundantPreprocessorCheck.cpp
+++ b/clang-tools-extra/clang-tidy/readability/RedundantPreprocessorCheck.cpp
@@ -36,10 +36,13 @@ class RedundantPreprocessorCallbacks : public PPCallbacks {
void If(SourceLocation Loc, SourceRange ConditionRange,
ConditionValueKind ConditionValue) override {
- const StringRef Condition =
- Lexer::getSourceText(CharSourceRange::getTokenRange(ConditionRange),
- PP.getSourceManager(), PP.getLangOpts());
- checkMacroRedundancy(Loc, Condition, IfStack, DK_If, DK_If, true);
+ // ConditionRange is unreliable for builtin preprocessor expressions like
+ // __has_builtin(...) because ExpandBuiltinMacro replaces the entire
+ // expression with a single token, collapsing the range. Extract the
+ // condition text directly from source instead.
+ const StringRef Condition = getConditionText(Loc);
+ if (!Condition.empty())
+ checkMacroRedundancy(Loc, Condition, IfStack, DK_If, DK_If, true);
}
void Ifdef(SourceLocation Loc, const Token &MacroNameTok,
@@ -69,6 +72,45 @@ class RedundantPreprocessorCallbacks : public PPCallbacks {
}
private:
+ /// Extract the condition text following the 'if' keyword from source.
+ /// Loc points to the 'if' keyword token, so we find its end and read
+ /// forward through the source buffer to the end of the directive,
+ /// handling backslash line continuations.
+ StringRef getConditionText(SourceLocation Loc) {
+ const SourceManager &SM = PP.getSourceManager();
+ const SourceLocation SpellingLoc = SM.getSpellingLoc(Loc);
+
+ const SourceLocation AfterIf =
+ Lexer::getLocForEndOfToken(SpellingLoc, 0, SM, PP.getLangOpts());
+ if (AfterIf.isInvalid())
+ return {};
+
+ bool Invalid = false;
+ auto [FID, Offset] = SM.getDecomposedLoc(AfterIf);
+ const StringRef Buffer = SM.getBufferData(FID, &Invalid);
+ if (Invalid || Offset >= Buffer.size())
+ return {};
+
+ // Read to end-of-line, handling backslash line continuations.
+ size_t End = Offset;
+ while (End < Buffer.size()) {
+ const char C = Buffer[End];
+ if (C == '\n' || C == '\r') {
+ if (End > Offset && Buffer[End - 1] == '\\') {
+ ++End;
+ if (End < Buffer.size() && Buffer[End - 1] == '\r' &&
+ Buffer[End] == '\n')
+ ++End;
+ continue;
+ }
+ break;
+ }
+ ++End;
+ }
+
+ return Buffer.slice(Offset, End).trim();
+ }
+
void checkMacroRedundancy(SourceLocation Loc, StringRef MacroName,
SmallVector<PreprocessorEntry, 4> &Stack,
DirectiveKind WarningKind, DirectiveKind NoteKind,
diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/redundant-preprocessor.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability/redundant-preprocessor.cpp
index 5429898e7ccd7..56e391733e4a0 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/readability/redundant-preprocessor.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/readability/redundant-preprocessor.cpp
@@ -82,3 +82,153 @@ void k();
void k();
#endif
#endif
+
+// Test case 1: Original bug report - different builtin checks should NOT trigger warning
+#if __has_builtin(__remove_cvref)
+# if __has_cpp_attribute(no_unique_address)
+# endif
+#endif
+
+// Test case 2: Actually redundant nested #if - SHOULD trigger warning
+#if defined(FOO)
+// CHECK-NOTES: [[@LINE+1]]:4: warning: nested redundant #if; consider removing it [readability-redundant-preprocessor]
+# if defined(FOO)
+// CHECK-NOTES: [[@LINE-3]]:2: note: previous #if was here
+# endif
+#endif
+
+// Test case 3: Different __has_builtin checks - should NOT trigger warning
+#if __has_builtin(__builtin_assume)
+# if __has_builtin(__builtin_expect)
+// These are different builtins, not redundant
+# endif
+#endif
+
+// Test case 4: Different __has_cpp_attribute checks - should NOT trigger warning
+#if __has_cpp_attribute(fallthrough)
+# if __has_cpp_attribute(nodiscard)
+// These are different attributes, not redundant
+# endif
+#endif
+
+// Test case 5: Same __has_builtin check - SHOULD trigger warning
+#if __has_builtin(__remove_cvref)
+// CHECK-NOTES: [[@LINE+1]]:4: warning: nested redundant #if; consider removing it [readability-redundant-preprocessor]
+# if __has_builtin(__remove_cvref)
+// CHECK-NOTES: [[@LINE-3]]:2: note: previous #if was here
+# endif
+#endif
+
+// Test case 6: Mixed conditions with __has_include - should NOT trigger warning
+#if __has_include(<vector>)
+# if __has_include(<string>)
+// Different include checks, not redundant
+# endif
+#endif
+
+// Test case 7: Complex expressions with logical operators - different conditions
+#if __has_builtin(__builtin_clz) && defined(__GNUC__)
+# if __has_builtin(__builtin_ctz) && defined(__GNUC__)
+# endif
+#endif
+
+// Test case 8: Same complex expression - SHOULD trigger warning
+#if __has_builtin(__builtin_clz) && defined(__GNUC__)
+// CHECK-NOTES: [[@LINE+1]]:4: warning: nested redundant #if; consider removing it [readability-redundant-preprocessor]
+# if __has_builtin(__builtin_clz) && defined(__GNUC__)
+// CHECK-NOTES: [[@LINE-3]]:2: note: previous #if was here
+# endif
+#endif
+
+#define MACRO1
+#define MACRO2
+
+// Test case 9: #ifdef tests - redundant SHOULD trigger
+#ifdef MACRO1
+// CHECK-NOTES: [[@LINE+1]]:4: warning: nested redundant #ifdef; consider removing it [readability-redundant-preprocessor]
+# ifdef MACRO1
+// CHECK-NOTES: [[@LINE-3]]:2: note: previous #ifdef was here
+# endif
+#endif
+
+// Test case 10: #ifdef tests - different macros should NOT trigger
+#ifdef MACRO1
+# ifdef MACRO2
+// Different macros, not redundant
+# endif
+#endif
+
+// Test case 11: #ifndef tests - redundant SHOULD trigger
+#ifndef MACRO3
+// CHECK-NOTES: [[@LINE+1]]:4: warning: nested redundant #ifndef; consider removing it [readability-redundant-preprocessor]
+# ifndef MACRO3
+// CHECK-NOTES: [[@LINE-3]]:2: note: previous #ifndef was here
+# endif
+#endif
+
+// Test case 12: #ifndef tests - different macros should NOT trigger
+#ifndef MACRO3
+# ifndef MACRO4
+// Different macros, not redundant
+# endif
+#endif
+
+// Test case 13: __has_feature checks - different features should NOT trigger
+#if __has_feature(cxx_rvalue_references)
+# if __has_feature(cxx_lambdas)
+// Different features, not redundant
+# endif
+#endif
+
+// Test case 14: Version checks - different versions should NOT trigger
+#if __cplusplus >= 201103L
+# if __cplusplus >= 201402L
+// Different version checks, not redundant (C++11 vs C++14)
+# endif
+#endif
+
+// Test case 15: Nested with multiple levels - only truly redundant should trigger
+#if __has_builtin(__type_pack_element)
+# if __has_cpp_attribute(no_unique_address)
+// CHECK-NOTES: [[@LINE+1]]:6: warning: nested redundant #if; consider removing it [readability-redundant-preprocessor]
+# if __has_builtin(__type_pack_element)
+// CHECK-NOTES: [[@LINE-4]]:2: note: previous #if was here
+# endif
+# endif
+#endif
+
+// Test case 16: Different builtin functions with similar names - should NOT trigger
+#if __has_builtin(__builtin_addressof)
+# if __has_builtin(__builtin_assume_aligned)
+// Different builtins, not redundant
+# endif
+#endif
+
+// Test case 17: Compiler version checks - should NOT trigger for different compilers
+#if defined(__clang__)
+# if defined(__GNUC__)
+// Different compilers, not redundant
+# endif
+#endif
+
+// Test case 18: Mix of __has_builtin and regular macro - should NOT trigger
+#if __has_builtin(__make_integer_seq)
+# if defined(USE_STD_INTEGER_SEQUENCE)
+// Different condition types, not redundant
+# endif
+#endif
+
+// Test case 19: Numeric comparisons - different values should NOT trigger
+#if __GNUC__ >= 2
+# if __GNUC__ >= 3
+// Different version checks, not redundant
+# endif
+#endif
+
+// Test case 20: Same numeric comparison - SHOULD trigger
+#if __GNUC__ >= 2
+// CHECK-NOTES: [[@LINE+1]]:4: warning: nested redundant #if; consider removing it [readability-redundant-preprocessor]
+# if __GNUC__ >= 2
+// CHECK-NOTES: [[@LINE-3]]:2: note: previous #if was here
+# endif
+#endif
\ No newline at end of file
More information about the cfe-commits
mailing list