https://github.com/mamadou-wane updated https://github.com/llvm/llvm-project/pull/225827
>From 85e4ce55114f93cc44c5a4bfa723b8f415efd399 Mon Sep 17 00:00:00 2001 From: Mamadou Wane <[email protected]> Date: Wed, 23 Sep 2026 11:34:50 -0400 Subject: [PATCH 1/2] [clang-tidy] Fix redundant-branch-condition false positive in loops Fixes #205685. Assisted-by: Claude --- .../RedundantBranchConditionCheck.cpp | 32 ++++ clang-tools-extra/docs/ReleaseNotes.md | 5 + .../bugprone/redundant-branch-condition.cpp | 164 ++++++++++++++++++ 3 files changed, 201 insertions(+) diff --git a/clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp index a6458d96055c3a..49cb316b63c5f4 100644 --- a/clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp @@ -10,6 +10,8 @@ #include "../utils/Aliasing.h" #include "../utils/LexerUtils.h" #include "clang/AST/ASTContext.h" +#include "clang/AST/ParentMapContext.h" +#include "clang/AST/StmtCXX.h" #include "clang/ASTMatchers/ASTMatchFinder.h" #include "clang/Analysis/Analyses/ExprMutationAnalyzer.h" #include "clang/Lex/Lexer.h" @@ -40,6 +42,29 @@ static bool isChangedBefore(const Stmt *S, const Stmt *NextS, const Stmt *PrevS, SM.isBeforeInTranslationUnit(MutS->getEndLoc(), NextS->getBeginLoc()); } +/// Returns the outermost loop that encloses `S` and is itself enclosed by +/// `Outer`, or null if there is no such loop. The walk passes through +/// declarations, such as a variable initialized by a lambda, but stops at the +/// enclosing function. +static const Stmt *getOutermostLoopBetween(const Stmt *S, const Stmt *Outer, + ASTContext *Context) { + const Stmt *Loop = nullptr; + DynTypedNodeList Parents = Context->getParents(*S); + while (!Parents.empty()) { + const DynTypedNode Parent = Parents[0]; + if (Parent.get<FunctionDecl>()) + break; + if (const auto *ParentStmt = Parent.get<Stmt>()) { + if (ParentStmt == Outer) + break; + if (isa<ForStmt, WhileStmt, DoStmt, CXXForRangeStmt>(ParentStmt)) + Loop = ParentStmt; + } + Parents = Context->getParents(Parent); + } + return Loop; +} + void RedundantBranchConditionCheck::registerMatchers(MatchFinder *Finder) { const auto ImmutableVar = varDecl(anyOf(parmVarDecl(), hasLocalStorage()), hasType(isInteger()), @@ -98,6 +123,13 @@ void RedundantBranchConditionCheck::check( return; } + // Inside a loop, a mutation anywhere in the loop runs before the inner + // condition is evaluated again, even if it comes later in the source. + const Stmt *Loop = getOutermostLoopBetween(InnerIf, OuterIf, Result.Context); + if (Loop && + ExprMutationAnalyzer(*Loop, *Result.Context).findMutation(CondVar)) + return; + // If the variable has an alias then it can be changed by that alias as well. // FIXME: could potentially support tracking pointers and references in the // future to improve catching true positives through aliases. diff --git a/clang-tools-extra/docs/ReleaseNotes.md b/clang-tools-extra/docs/ReleaseNotes.md index 833638a47abc63..3d9e34cc4d44e7 100644 --- a/clang-tools-extra/docs/ReleaseNotes.md +++ b/clang-tools-extra/docs/ReleaseNotes.md @@ -185,6 +185,11 @@ infrastructure are described first, followed by tool-specific sections. <clang-tidy/checks/bugprone/pointer-arithmetic-on-polymorphic-object>` when the pointer points to an incomplete (forward-declared) type. +- Improved {doc}`bugprone-redundant-branch-condition + <clang-tidy/checks/bugprone/redundant-branch-condition>` check by fixing + false positives when the condition variable is changed later in a loop that + encloses the inner `if`. + - Fixed a crash in {doc}`bugprone-std-namespace-modification <clang-tidy/checks/bugprone/std-namespace-modification>` when checking lambda closure types used as template arguments. diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/redundant-branch-condition.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/redundant-branch-condition.cpp index 40994b0ff884eb..ad2238ff9984f3 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/redundant-branch-condition.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/redundant-branch-condition.cpp @@ -1127,6 +1127,52 @@ int positive_expr_with_cleanups() { return 0; } +// Loops + +void positive_loop_not_mutated() { + bool onFire = isBurning(); + if (onFire) { + while (someOtherCondition()) { + if (onFire) { + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: redundant condition 'onFire' [bugprone-redundant-branch-condition] + // CHECK-FIXES: {{^\ *$}} + scream(); + } + // CHECK-FIXES: {{^\ *$}} + } + } +} + +void positive_loop_mutated_after_loop() { + bool onFire = isBurning(); + if (onFire) { + while (someOtherCondition()) { + if (onFire) { + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: redundant condition 'onFire' [bugprone-redundant-branch-condition] + // CHECK-FIXES: {{^\ *$}} + scream(); + } + // CHECK-FIXES: {{^\ *$}} + } + tryToExtinguish(onFire); + } +} + +void positive_loop_around_both_ifs() { + bool onFire = isBurning(); + while (someOtherCondition()) { + if (onFire) { + if (onFire) { + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: redundant condition 'onFire' [bugprone-redundant-branch-condition] + // CHECK-FIXES: {{^\ *$}} + scream(); + } + // CHECK-FIXES: {{^\ *$}} + } + tryToExtinguish(onFire); + } +} + //===--- Special Negatives ------------------------------------------------===// // Aliasing @@ -1351,6 +1397,109 @@ void negative_comma_after_condition() { } } +// Loops + +void negative_for_mutated_later_in_body(int n) { + bool onFire = isBurning(); + if (onFire) { + for (int i = 0; i < n; ++i) { + switch (i) { + case 4: + case 5: + if (onFire) { + // NO-MESSAGE: fire may have been extinguished in a previous iteration + onFire = false; + scream(); + } + break; + } + } + } +} + +void negative_while_mutated_later_in_body() { + bool onFire = isBurning(); + if (onFire) { + while (someOtherCondition()) { + if (onFire) { + // NO-MESSAGE: fire may have been extinguished in a previous iteration + scream(); + } + tryToExtinguish(onFire); + } + } +} + +void negative_do_mutated_later_in_body() { + bool onFire = isBurning(); + if (onFire) { + do { + if (onFire) { + // NO-MESSAGE: fire may have been extinguished in a previous iteration + scream(); + } + onFire = isBurning(); + } while (someOtherCondition()); + } +} + +void negative_range_for_mutated_later_in_body() { + bool onFire = isBurning(); + int floors[3] = {1, 2, 3}; + if (onFire) { + for (int floor : floors) { + if (onFire) { + // NO-MESSAGE: fire may have been extinguished in a previous iteration + scream(); + } + onFire = floor > 1; + } + } +} + +void negative_loop_condition_mutates() { + bool onFire = isBurning(); + if (onFire) { + do { + if (onFire) { + // NO-MESSAGE: fire may have been extinguished by the loop condition + scream(); + } + } while (tryToExtinguish(onFire)); + } +} + +void negative_loop_mutated_after_lambda_variable() { + bool onFire = isBurning(); + if (onFire) { + while (someOtherCondition()) { + auto check = [onFire] { + if (onFire) { + // NO-MESSAGE: fire may have been extinguished in a previous iteration + scream(); + } + }; + check(); + tryToExtinguish(onFire); + } + } +} + +void negative_mutated_in_outer_loop() { + bool onFire = isBurning(); + if (onFire) { + while (someOtherCondition()) { + for (int i = 0; i < 3; ++i) { + if (onFire) { + // NO-MESSAGE: fire may have been extinguished in a previous iteration + scream(); + } + } + tryToExtinguish(onFire); + } + } +} + //===--- Unhandled Cases --------------------------------------------------===// void negated_in_else() { @@ -1397,3 +1546,18 @@ void volatile_concrete_address() { } } } + +void loop_mutated_then_break() { + bool onFire = isBurning(); + if (onFire) { + while (someOtherCondition()) { + if (onFire) { + // Redundant, but not diagnosed: the loop exits before onFire is checked + // again. Telling this apart from a later mutation needs the CFG. + onFire = false; + scream(); + break; + } + } + } +} >From c19d0254d40b9d177106a2656a5bf44212b5886d Mon Sep 17 00:00:00 2001 From: Mamadou Wane <[email protected]> Date: Thu, 24 Sep 2026 09:46:34 -0400 Subject: [PATCH 2/2] [clang-tidy] Clarify the getParents() walk Assisted-by: Claude --- .../clang-tidy/bugprone/RedundantBranchConditionCheck.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp index 49cb316b63c5f4..8d608dd4b0e666 100644 --- a/clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp @@ -49,6 +49,8 @@ static bool isChangedBefore(const Stmt *S, const Stmt *NextS, const Stmt *PrevS, static const Stmt *getOutermostLoopBetween(const Stmt *S, const Stmt *Outer, ASTContext *Context) { const Stmt *Loop = nullptr; + // getParents() returns only the direct parents of a node, usually exactly + // one, so the walk calls it once per level. DynTypedNodeList Parents = Context->getParents(*S); while (!Parents.empty()) { const DynTypedNode Parent = Parents[0]; _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
