[clang-tools-extra] [clang-tidy] Fix redundant-branch-condition false positive in loops (PR #225827)
Mamadou Wane via cfe-commits
cfe-commits at lists.llvm.org
Thu Sep 24 06:46:50 PDT 2026
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 <mamadouswane at gmail.com>
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 <mamadouswane at gmail.com>
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];
More information about the cfe-commits
mailing list