https://github.com/Aditya26189 updated https://github.com/llvm/llvm-project/pull/181734
>From 36277411068768852abd8c660d5f6571d504b5d8 Mon Sep 17 00:00:00 2001 From: Aditya Singh <[email protected]> Date: Tue, 17 Feb 2026 01:17:44 +0530 Subject: [PATCH 1/2] [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 | 49 +++++++- clang-tools-extra/docs/ReleaseNotes.rst | 5 + .../readability/redundant-preprocessor.cpp | 113 ++++++++++++++++++ 3 files changed, 163 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..8b4342218dd70 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,44 @@ 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 == '\r' || C == '\n') { + if (End > Offset && Buffer[End - 1] == '\\') { + ++End; + if (C == '\r' && End < Buffer.size() && 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/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index ee057d9bf0444..8f14c544b501c 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -236,6 +236,11 @@ Changes in existing checks positives on parameters used in dependent expressions (e.g. inside generic lambdas). +- Improved :doc:`readability-redundant-preprocessor + <clang-tidy/checks/readability/redundant-preprocessor>` check by fixing a + false positive for nested ``#if`` directives using different builtin + expressions such as ``__has_builtin`` and ``__has_cpp_attribute``. + - Improved :doc:`readability-simplify-boolean-expr <clang-tidy/checks/readability/simplify-boolean-expr>` check to provide valid fix suggestions for C23 and later by not using ``static_cast``. 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..fbab94f9756bc 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,116 @@ void k(); void k(); #endif #endif + +// Different builtin checks should NOT trigger warning +#if __has_builtin(__remove_cvref) +# if __has_cpp_attribute(no_unique_address) +# endif +#endif + +// Redundant nested #if +#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 + +// Different __has_builtin checks +#if __has_builtin(__builtin_assume) +# if __has_builtin(__builtin_expect) +# endif +#endif + +// Different __has_cpp_attribute checks +#if __has_cpp_attribute(fallthrough) +# if __has_cpp_attribute(nodiscard) +# endif +#endif + +// Same __has_builtin check +#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 + +// Different __has_include checks +#if __has_include(<vector>) +# if __has_include(<string>) +# endif +#endif + +// Complex expressions - different conditions +#if __has_builtin(__builtin_clz) && defined(__GNUC__) +# if __has_builtin(__builtin_ctz) && defined(__GNUC__) +# endif +#endif + +#define MACRO1 +#define MACRO2 + +// Redundant #ifdef +#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 + +// Different macros - #ifdef +#ifdef MACRO1 +# ifdef MACRO2 +# endif +#endif + +// Redundant #ifndef +#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 + +// Different macros - #ifndef +#ifndef MACRO3 +# ifndef MACRO4 +# endif +#endif + +// Different __has_feature checks +#if __has_feature(cxx_rvalue_references) +# if __has_feature(cxx_lambdas) +# endif +#endif + +// Different version checks +#if __cplusplus >= 201103L +# if __cplusplus >= 201402L +# endif +#endif + +// Different builtin functions with similar names +#if __has_builtin(__builtin_addressof) +# if __has_builtin(__builtin_assume_aligned) +# endif +#endif + +// Different compiler checks +#if defined(__clang__) +# if defined(__GNUC__) +# endif +#endif + +// Mixed builtin and regular macro +#if __has_builtin(__make_integer_seq) +# if defined(USE_STD_INTEGER_SEQUENCE) +# endif +#endif + +// Different numeric comparisons +#if __GNUC__ >= 2 +# if __GNUC__ >= 3 +# endif +#endif >From ab0e578289f84a67b69218fdefb9d50145c8951e Mon Sep 17 00:00:00 2001 From: Aditya Singh <[email protected]> Date: Wed, 18 Feb 2026 19:27:23 +0530 Subject: [PATCH 2/2] address review: add comment test cases --- .../RedundantPreprocessorCheck.cpp | 84 +++++++++---------- .../readability/redundant-preprocessor.cpp | 40 +++++++++ 2 files changed, 79 insertions(+), 45 deletions(-) diff --git a/clang-tools-extra/clang-tidy/readability/RedundantPreprocessorCheck.cpp b/clang-tools-extra/clang-tidy/readability/RedundantPreprocessorCheck.cpp index 8b4342218dd70..7dbd82f4b0c95 100644 --- a/clang-tools-extra/clang-tidy/readability/RedundantPreprocessorCheck.cpp +++ b/clang-tools-extra/clang-tidy/readability/RedundantPreprocessorCheck.cpp @@ -15,6 +15,42 @@ namespace clang::tidy::readability { namespace { + +static StringRef getConditionText(SourceLocation Loc, const SourceManager &SM, + const LangOptions &LangOpts) { + bool Invalid = false; + FileID FID = SM.getFileID(Loc); + StringRef Buffer = SM.getBufferData(FID, &Invalid); + if (Invalid) + return {}; + + // Initialize a raw lexer starting exactly at the condition's location + Lexer RawLexer(SM.getLocForStartOfFile(FID), LangOpts, Buffer.begin(), + SM.getCharacterData(Loc), Buffer.end()); + RawLexer.SetCommentRetentionState(true); + + Token Tok; + // Lex the 'if' token itself + RawLexer.LexFromRawLexer(Tok); + + unsigned StartOffset = SM.getFileOffset(Tok.getEndLoc()); + unsigned EndOffset = StartOffset; + + // Lex tokens until we hit the start of a new line or EOF. + // The lexer handles backslash line continuations automatically. + while (!RawLexer.LexFromRawLexer(Tok)) { + if (Tok.isAtStartOfLine() || Tok.is(tok::eof)) + break; + EndOffset = SM.getFileOffset(Tok.getLocation()) + Tok.getLength(); + } + + if (EndOffset <= StartOffset) + return {}; + + // Extract the raw text from the buffer to preserve original spacing + return Buffer.substr(StartOffset, EndOffset - StartOffset).trim(); +} + /// Information about an opening preprocessor directive. struct PreprocessorEntry { SourceLocation Loc; @@ -36,13 +72,9 @@ class RedundantPreprocessorCallbacks : public PPCallbacks { void If(SourceLocation Loc, SourceRange ConditionRange, ConditionValueKind ConditionValue) override { - // 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); + const StringRef Condition = + getConditionText(Loc, PP.getSourceManager(), PP.getLangOpts()); + checkMacroRedundancy(Loc, Condition, IfStack, DK_If, DK_If, true); } void Ifdef(SourceLocation Loc, const Token &MacroNameTok, @@ -72,44 +104,6 @@ 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 == '\r' || C == '\n') { - if (End > Offset && Buffer[End - 1] == '\\') { - ++End; - if (C == '\r' && End < Buffer.size() && 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 fbab94f9756bc..72c05bae1bac2 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 @@ -195,3 +195,43 @@ void k(); # if __GNUC__ >= 3 # endif #endif + +// Test: inline comments - same condition and comment SHOULD warn +#if __has_builtin(__remove_cvref) // same comment +// CHECK-NOTES: [[@LINE+1]]:4: warning: nested redundant #if; consider removing it [readability-redundant-preprocessor] +# if __has_builtin(__remove_cvref) // same comment +// CHECK-NOTES: [[@LINE-3]]:2: note: previous #if was here +# endif +#endif + +// Test: inline comments - different comments, condition text differs, no warn +#if defined(FOO) // comment one +# if defined(FOO) // comment two +# endif +#endif + +// Test: block comments - same condition and comment SHOULD warn +#if __has_builtin(__remove_cvref) /* block */ +// CHECK-NOTES: [[@LINE+1]]:4: warning: nested redundant #if; consider removing it [readability-redundant-preprocessor] +# if __has_builtin(__remove_cvref) /* block */ +// CHECK-NOTES: [[@LINE-3]]:2: note: previous #if was here +# endif +#endif + +// Test: multiline block comment spanning lines - same condition SHOULD warn +#if __has_builtin(__remove_cvref) /* multiline + comment */ +// CHECK-NOTES: [[@LINE+2]]:4: warning: nested redundant #if; consider removing it [readability-redundant-preprocessor] +// CHECK-NOTES: [[@LINE-3]]:2: note: previous #if was here +# if __has_builtin(__remove_cvref) /* multiline + comment */ +# endif +#endif + +// Test: multiline block comment - different conditions should NOT warn +#if __has_builtin(__remove_cvref) /* multiline + comment */ +# if __has_cpp_attribute(no_unique_address) /* multiline + comment */ +# endif +#endif _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
