Author: Mamadou Wane Date: 2026-09-27T18:18:14+08:00 New Revision: dfde6ac69e6e0c5a717042de3fa7f118eea7d830
URL: https://github.com/llvm/llvm-project/commit/dfde6ac69e6e0c5a717042de3fa7f118eea7d830 DIFF: https://github.com/llvm/llvm-project/commit/dfde6ac69e6e0c5a717042de3fa7f118eea7d830.diff LOG: [clang-tidy] Fix redundant-branch-condition false positive in loops (#225827) Fixes #205685. `bugprone-redundant-branch-condition` decides whether the condition variable changes between the outer and inner `if` by comparing source positions. When a loop sits between the two, a mutation that comes after the inner `if` in the source still runs before the inner condition is evaluated again on the next iteration, so the check reports a redundant condition that isn't, and the fix-it changes behavior. The fix walks the parents of the inner `if` up to the outer `if` and finds the outermost enclosing loop. If the variable is mutated anywhere in that loop, the check does not warn. This follows NagyDonat's suggestion in the issue: the existing check already covers mutations between the outer condition and the loop, so only the loop itself needs the extra query. The walk passes through declarations, so an inner `if` inside a lambda stored in a variable is handled too, and it stops at the enclosing function. I chose a syntactic walk instead of the CFG (`utils::ExprSequence`), since it covers the range the issue asks for without building a CFG on every match. Tests cover `for`, `while`, `do`, and range-based `for`, a mutation in a `do`/`while` condition, a lambda stored in a variable inside the loop, and an inner loop whose enclosing loop does the mutation. Each of these warns without the patch. Three positive cases confirm the warning is kept when the loop never mutates the variable, when the mutation comes after the loop, and when the loop encloses both `if` statements. Limitations: - Any mutation in the loop suppresses the warning, including one followed by an exit from the loop (`break`, `return`, `throw`) or one that cannot make the condition false. The inner condition is still redundant in those cases; telling them apart needs the CFG. I added the `break` case under Unhandled Cases. The change only removes warnings, so it adds no new false positives. - Loops built with `goto` and Objective-C `for ... in` loops are not recognized, so the false positive remains there. I used Claude to help write the code. I tested and reviewed all of it myself, and worked through the approach and suggestions with Claude. Added: Modified: clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp clang-tools-extra/docs/ReleaseNotes.md clang-tools-extra/test/clang-tidy/checkers/bugprone/redundant-branch-condition.cpp Removed: ################################################################################ diff --git a/clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp index a6458d96055c3..8d608dd4b0e66 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,31 @@ 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; + // 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]; + 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 +125,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 833638a47abc6..3d9e34cc4d44e 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 40994b0ff884e..ad2238ff9984f 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; + } + } + } +} _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
