[clang-tools-extra] dfde6ac - [clang-tidy] Fix redundant-branch-condition false positive in loops (#225827)
via cfe-commits
cfe-commits at lists.llvm.org
Sun Sep 27 03:18:20 PDT 2026
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;
+ }
+ }
+ }
+}
More information about the cfe-commits
mailing list