https://github.com/flowerhack created
https://github.com/llvm/llvm-project/pull/155731
…opy.
Adds an option to performance-for-range-copy that alerts when a loop variable
is copied and modified.
This is a bugprone pattern because the programmer in this case often assumes
they are modifying the original value instead of a copy.
The warning can be suppressed by instead explicitly copying the value inside
the body of the loop.
>From 06b93e18274fa727e73435b3b03882963f802ae7 Mon Sep 17 00:00:00 2001
From: Julia Hansbrough <flowerh...@google.com>
Date: Fri, 15 Aug 2025 21:48:25 +0000
Subject: [PATCH] Add WarnOnModificationOfCopiedLoopVariable to
performance-for-range-copy.
Adds an option to performance-for-range-copy that alerts when a loop
variable is copied and modified.
This is a bugprone pattern because the programmer in this case often
assumes they are modifying the original value instead of a copy.
The warning can be suppressed by instead explicitly copying the value
inside the body of the loop.
---
.../performance/ForRangeCopyCheck.cpp | 32 +++++++++++-
.../performance/ForRangeCopyCheck.h | 8 +++
.../checks/performance/for-range-copy.rst | 12 +++++
...e-warn-on-copied-loop-variable-mutated.cpp | 49 +++++++++++++++++++
4 files changed, 99 insertions(+), 2 deletions(-)
create mode 100644
clang-tools-extra/test/clang-tidy/checkers/performance/for-range-warn-on-copied-loop-variable-mutated.cpp
diff --git a/clang-tools-extra/clang-tidy/performance/ForRangeCopyCheck.cpp
b/clang-tools-extra/clang-tidy/performance/ForRangeCopyCheck.cpp
index f545a49dc184b..cbb842927bd57 100644
--- a/clang-tools-extra/clang-tidy/performance/ForRangeCopyCheck.cpp
+++ b/clang-tools-extra/clang-tidy/performance/ForRangeCopyCheck.cpp
@@ -22,11 +22,15 @@ namespace clang::tidy::performance {
ForRangeCopyCheck::ForRangeCopyCheck(StringRef Name, ClangTidyContext *Context)
: ClangTidyCheck(Name, Context),
WarnOnAllAutoCopies(Options.get("WarnOnAllAutoCopies", false)),
+ WarnOnModificationOfCopiedLoopVariable(
+ Options.get("WarnOnModificationOfCopiedLoopVariable", false)),
AllowedTypes(
utils::options::parseStringList(Options.get("AllowedTypes", ""))) {}
void ForRangeCopyCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
Options.store(Opts, "WarnOnAllAutoCopies", WarnOnAllAutoCopies);
+ Options.store(Opts, "WarnOnModificationOfCopiedLoopVariable",
+ WarnOnModificationOfCopiedLoopVariable);
Options.store(Opts, "AllowedTypes",
utils::options::serializeStringList(AllowedTypes));
}
@@ -64,9 +68,11 @@ void ForRangeCopyCheck::check(const MatchFinder::MatchResult
&Result) {
// Ignore code in macros since we can't place the fixes correctly.
if (Var->getBeginLoc().isMacroID())
return;
+ const auto *ForRange = Result.Nodes.getNodeAs<CXXForRangeStmt>("forRange");
+ if (copiedLoopVarIsMutated(*Var, *ForRange, *Result.Context))
+ return;
if (handleConstValueCopy(*Var, *Result.Context))
return;
- const auto *ForRange = Result.Nodes.getNodeAs<CXXForRangeStmt>("forRange");
handleCopyIsOnlyConstReferenced(*Var, *ForRange, *Result.Context);
}
@@ -79,6 +85,7 @@ bool ForRangeCopyCheck::handleConstValueCopy(const VarDecl
&LoopVar,
} else if (!LoopVar.getType().isConstQualified()) {
return false;
}
+
std::optional<bool> Expensive =
utils::type_traits::isExpensiveToCopy(LoopVar.getType(), Context);
if (!Expensive || !*Expensive)
@@ -105,6 +112,27 @@ static bool isReferenced(const VarDecl &LoopVar, const
Stmt &Stmt,
.empty();
}
+bool ForRangeCopyCheck::copiedLoopVarIsMutated(const VarDecl &LoopVar,
+ const CXXForRangeStmt &ForRange,
+ ASTContext &Context) {
+ // If it's copied and mutated, there's a high chance that's a bug.
+ if (WarnOnModificationOfCopiedLoopVariable) {
+ if (ExprMutationAnalyzer(*ForRange.getBody(), Context)
+ .isMutated(&LoopVar)) {
+ auto Diag =
+ diag(LoopVar.getLocation(), "loop variable is copied and then "
+ "modified, which is likely a bug; you "
+ "probably want to modify the underlying "
+ "object and not this copy. If you "
+ "*did* intend to modify this copy, "
+ "please use an explicit copy inside the "
+ "body of the loop");
+ return true;
+ }
+ }
+ return false;
+}
+
bool ForRangeCopyCheck::handleCopyIsOnlyConstReferenced(
const VarDecl &LoopVar, const CXXForRangeStmt &ForRange,
ASTContext &Context) {
@@ -112,6 +140,7 @@ bool ForRangeCopyCheck::handleCopyIsOnlyConstReferenced(
utils::type_traits::isExpensiveToCopy(LoopVar.getType(), Context);
if (LoopVar.getType().isConstQualified() || !Expensive || !*Expensive)
return false;
+
// We omit the case where the loop variable is not used in the loop body.
E.g.
//
// for (auto _ : benchmark_state) {
@@ -130,7 +159,6 @@ bool ForRangeCopyCheck::handleCopyIsOnlyConstReferenced(
if (std::optional<FixItHint> Fix = utils::fixit::addQualifierToVarDecl(
LoopVar, Context, Qualifiers::Const))
Diag << *Fix << utils::fixit::changeVarDeclToReference(LoopVar, Context);
-
return true;
}
return false;
diff --git a/clang-tools-extra/clang-tidy/performance/ForRangeCopyCheck.h
b/clang-tools-extra/clang-tidy/performance/ForRangeCopyCheck.h
index 8fabbfa2ae7ba..722ca2f3a2e24 100644
--- a/clang-tools-extra/clang-tidy/performance/ForRangeCopyCheck.h
+++ b/clang-tools-extra/clang-tidy/performance/ForRangeCopyCheck.h
@@ -39,8 +39,16 @@ class ForRangeCopyCheck : public ClangTidyCheck {
const CXXForRangeStmt &ForRange,
ASTContext &Context);
+ // Checks if the loop variable is copied and then subsequently mutated
+ // in the body of the loop. If so it suggests the copy was unintentional,
+ // or, that the copy would be more clear if done inside the body of the loop.
+ bool copiedLoopVarIsMutated(const VarDecl &LoopVar,
+ const CXXForRangeStmt &ForRange,
+ ASTContext &Context);
+
const bool WarnOnAllAutoCopies;
const std::vector<StringRef> AllowedTypes;
+ const bool WarnOnModificationOfCopiedLoopVariable;
};
} // namespace clang::tidy::performance
diff --git
a/clang-tools-extra/docs/clang-tidy/checks/performance/for-range-copy.rst
b/clang-tools-extra/docs/clang-tidy/checks/performance/for-range-copy.rst
index d740984029482..a8dbf6e3d435a 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/performance/for-range-copy.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/performance/for-range-copy.rst
@@ -18,6 +18,12 @@ following heuristic is employed:
invoked on it, or it is used as const reference or value argument in
constructors or function calls.
+There is an additional option to warn any time a loop variable is copied in
each
+iteration and subsequently modified in the body of the loop. This is likely to
+be a bug, since the user may believe they are modifying the underlying value
+instead of a copy, and a user that truly intends to modify a copy may do so by
+performing the copy explicitly inside the body of the loop.
+
Options
-------
@@ -26,6 +32,12 @@ Options
When `true`, warns on any use of ``auto`` as the type of the range-based for
loop variable. Default is `false`.
+.. option:: WarnOnModificationOfCopiedLoopVariable
+
+ When `true`, warns when the loop variable is copied and subsequently
+ modified. This is likely to be a bug. Moving the copy to an explicit copy
+ inside of the loop will suppress the warning. Default is `false`.
+
.. option:: AllowedTypes
A semicolon-separated list of names of types allowed to be copied in each
diff --git
a/clang-tools-extra/test/clang-tidy/checkers/performance/for-range-warn-on-copied-loop-variable-mutated.cpp
b/clang-tools-extra/test/clang-tidy/checkers/performance/for-range-warn-on-copied-loop-variable-mutated.cpp
new file mode 100644
index 0000000000000..ed34a4fc586c0
--- /dev/null
+++
b/clang-tools-extra/test/clang-tidy/checkers/performance/for-range-warn-on-copied-loop-variable-mutated.cpp
@@ -0,0 +1,49 @@
+// RUN: %check_clang_tidy %s performance-for-range-copy %t -- \
+// RUN: -config="{CheckOptions:
{performance-for-range-copy.WarnOnModificationOfCopiedLoopVariable: true}}"
+
+template <typename T>
+struct Iterator {
+ void operator++() {}
+ const T& operator*() {
+ static T* TT = new T();
+ return *TT;
+ }
+ bool operator!=(const Iterator &) { return false; }
+};
+template <typename T>
+struct View {
+ T begin() { return T(); }
+ T begin() const { return T(); }
+ T end() { return T(); }
+ T end() const { return T(); }
+};
+
+struct S {
+ int value;
+
+ S() : value(0) {};
+ S(const S &);
+ ~S();
+ S &operator=(const S &);
+ void modify() {
+ value++;
+ }
+};
+
+void NegativeLoopVariableNotCopied() {
+ for (const S& S1 : View<Iterator<S>>()) {
+ }
+}
+
+void NegativeLoopVariableCopiedButNotModified() {
+ for (S S1 : View<Iterator<S>>()) {
+ }
+}
+
+void PositiveLoopVariableCopiedAndThenModfied() {
+ for (S S1 : View<Iterator<S>>()) {
+ // CHECK-MESSAGES: [[@LINE-1]]:10: warning: loop variable is copied and
then modified, which is likely a bug; you probably want to modify the
underlying object and not this copy. If you *did* intend to modify this copy,
please use an explicit copy inside the body of the loop
+ S1.modify();
+ }
+}
+
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
