[clang] [clang-tools-extra] [clang-tidy] Handle C++ structured bindings in `performance-for-range-copy` (PR #77105)
Clement Courbet via cfe-commits
cfe-commits at lists.llvm.org
Fri Jan 5 07:07:01 PST 2024
https://github.com/legrosbuffle created https://github.com/llvm/llvm-project/pull/77105
Right now we are not triggering on:
```
for (auto [x, y] : container) {
// const-only access
}
```
>From d5b83c7e8624ba6fde2ea9eb8c3589fe083067b8 Mon Sep 17 00:00:00 2001
From: Clement Courbet <courbet at google.com>
Date: Fri, 5 Jan 2024 15:48:51 +0100
Subject: [PATCH] [clang-tidy] Handle C++ bindings in
`performance-for-range-copy`
Right now we are not triggering on:
```
for (auto [x, y] : container) {
// const-only access
}
```
---
.../performance/ForRangeCopyCheck.cpp | 13 ++++++--
clang-tools-extra/docs/ReleaseNotes.rst | 4 +++
.../checkers/performance/for-range-copy.cpp | 31 ++++++++++++++++---
clang/lib/Analysis/ExprMutationAnalyzer.cpp | 13 ++++++--
.../Analysis/ExprMutationAnalyzerTest.cpp | 20 ++++++++++++
5 files changed, 70 insertions(+), 11 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/performance/ForRangeCopyCheck.cpp b/clang-tools-extra/clang-tidy/performance/ForRangeCopyCheck.cpp
index 5bfa6fb0d02d5c..655e480ffa62cb 100644
--- a/clang-tools-extra/clang-tidy/performance/ForRangeCopyCheck.cpp
+++ b/clang-tools-extra/clang-tidy/performance/ForRangeCopyCheck.cpp
@@ -97,6 +97,15 @@ bool ForRangeCopyCheck::handleConstValueCopy(const VarDecl &LoopVar,
return true;
}
+static bool isReferenced(const VarDecl &LoopVar, const Stmt &Stmt,
+ ASTContext &Context) {
+ const auto IsLoopVar = varDecl(equalsNode(&LoopVar));
+ return !match(stmt(hasDescendant(declRefExpr(to(valueDecl(anyOf(
+ IsLoopVar, bindingDecl(forDecomposition(IsLoopVar)))))))),
+ Stmt, Context)
+ .empty();
+}
+
bool ForRangeCopyCheck::handleCopyIsOnlyConstReferenced(
const VarDecl &LoopVar, const CXXForRangeStmt &ForRange,
ASTContext &Context) {
@@ -113,9 +122,7 @@ bool ForRangeCopyCheck::handleCopyIsOnlyConstReferenced(
// compiler warning which can't be suppressed.
// Since this case is very rare, it is safe to ignore it.
if (!ExprMutationAnalyzer(*ForRange.getBody(), Context).isMutated(&LoopVar) &&
- !utils::decl_ref_expr::allDeclRefExprs(LoopVar, *ForRange.getBody(),
- Context)
- .empty()) {
+ isReferenced(LoopVar, *ForRange.getBody(), Context)) {
auto Diag = diag(
LoopVar.getLocation(),
"loop variable is copied but only used as const reference; consider "
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 4d25e2ebe85f5f..4491d239dc7888 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -419,6 +419,10 @@ Changes in existing checks
<clang-tidy/checks/performance/faster-string-find>` check to properly escape
single quotes.
+- Improved :doc:`performance-for-range-copy
+ <clang-tidy/checks/performance/for-range-copy>` check to handle cases where
+ the loop variable is a structured binding.
+
- Improved :doc:`performance-noexcept-move-constructor
<clang-tidy/checks/performance/noexcept-move-constructor>` to better handle
conditional noexcept expressions, eliminating false-positives.
diff --git a/clang-tools-extra/test/clang-tidy/checkers/performance/for-range-copy.cpp b/clang-tools-extra/test/clang-tidy/checkers/performance/for-range-copy.cpp
index 1a2eedc9e65c53..e8c967dacfb7d5 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/performance/for-range-copy.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/performance/for-range-copy.cpp
@@ -47,6 +47,11 @@ struct S {
S &operator=(const S &);
};
+struct Point {
+ ~Point() {}
+ int x, y;
+};
+
struct Convertible {
operator S() const {
return S();
@@ -87,6 +92,10 @@ void instantiated() {
// CHECK-MESSAGES: [[@LINE-1]]:16: warning: the loop variable's type is {{.*}}
// CHECK-FIXES: {{^}} for (const S& S2 : View<Iterator<S>>()) {}
+ for (const auto [X, Y] : View<Iterator<Point>>()) {}
+ // CHECK-MESSAGES: [[@LINE-1]]:19: warning: the loop variable's type is
+ // CHECK-FIXES: {{^}} for (const auto& [X, Y] : View<Iterator<Point>>()) {}
+
for (const T T2 : View<Iterator<T>>()) {}
// CHECK-MESSAGES: [[@LINE-1]]:16: warning: the loop variable's type is {{.*}}
// CHECK-FIXES: {{^}} for (const T& T2 : View<Iterator<T>>()) {}
@@ -123,11 +132,6 @@ struct Mutable {
~Mutable() {}
};
-struct Point {
- ~Point() {}
- int x, y;
-};
-
Mutable& operator<<(Mutable &Out, bool B) {
Out.setBool(B);
return Out;
@@ -144,6 +148,7 @@ void useByValue(Mutable M);
void useByConstValue(const Mutable M);
void mutate(Mutable *M);
void mutate(Mutable &M);
+void mutate(int &);
void onceConstOnceMutated(const Mutable &M1, Mutable &M2);
void negativeVariableIsMutated() {
@@ -234,6 +239,22 @@ void positiveOnlyAccessedFieldAsConst() {
}
}
+void positiveOnlyAccessedFieldAsConstBinding() {
+ for (auto [X, Y] : View<Iterator<Point>>()) {
+ // CHECK-MESSAGES: [[@LINE-1]]:13: warning: loop variable is copied but
+ // CHECK-FIXES: for (const auto& [X, Y] : View<Iterator<Point>>()) {
+ use(X);
+ use(Y);
+ }
+}
+
+void negativeOnlyAccessedFieldAsConstBinding() {
+ for (auto [X, Y] : View<Iterator<Point>>()) {
+ use(X);
+ mutate(Y);
+ }
+}
+
void positiveOnlyUsedInCopyConstructor() {
for (auto A : View<Iterator<Mutable>>()) {
// CHECK-MESSAGES: [[@LINE-1]]:13: warning: loop variable is copied but only used as const reference; consider making it a const reference [performance-for-range-copy]
diff --git a/clang/lib/Analysis/ExprMutationAnalyzer.cpp b/clang/lib/Analysis/ExprMutationAnalyzer.cpp
index 624a643cc60e4b..29f05b25bdec2b 100644
--- a/clang/lib/Analysis/ExprMutationAnalyzer.cpp
+++ b/clang/lib/Analysis/ExprMutationAnalyzer.cpp
@@ -204,9 +204,16 @@ const Stmt *ExprMutationAnalyzer::findMutationMemoized(
const Stmt *ExprMutationAnalyzer::tryEachDeclRef(const Decl *Dec,
MutationFinder Finder) {
- const auto Refs =
- match(findAll(declRefExpr(to(equalsNode(Dec))).bind(NodeID<Expr>::value)),
- Stm, Context);
+ const auto Refs = match(
+ findAll(
+ declRefExpr(to(
+ // `Dec` or a binding if `Dec` is a decomposition.
+ anyOf(equalsNode(Dec),
+ bindingDecl(forDecomposition(equalsNode(Dec))))
+ //
+ ))
+ .bind(NodeID<Expr>::value)),
+ Stm, Context);
for (const auto &RefNodes : Refs) {
const auto *E = RefNodes.getNodeAs<Expr>(NodeID<Expr>::value);
if ((this->*Finder)(E))
diff --git a/clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp b/clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp
index f1a1f857a0ee5b..a94f857720b035 100644
--- a/clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp
+++ b/clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp
@@ -592,6 +592,26 @@ TEST(ExprMutationAnalyzerTest, ByConstRRefArgument) {
ElementsAre("static_cast<A &&>(x)"));
}
+// Section: bindings.
+
+TEST(ExprMutationAnalyzerTest, BindingModifies) {
+ const auto AST =
+ buildASTFromCode("struct Point { int x; int y; };"
+ "void mod(int&);"
+ "void f(Point p) { auto& [x, y] = p; mod(x); }");
+ const auto Results =
+ match(withEnclosingCompound(declRefTo("p")), AST->getASTContext());
+ EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("x", "mod(x)"));
+}
+
+TEST(ExprMutationAnalyzerTest, BindingDoesNotModify) {
+ const auto AST = buildASTFromCode("struct Point { int x; int y; };"
+ "void f(Point p) { auto& [x, y] = p; x; }");
+ const auto Results =
+ match(withEnclosingCompound(declRefTo("p")), AST->getASTContext());
+ EXPECT_FALSE(isMutated(Results, AST.get()));
+}
+
// section: explicit std::move and std::forward testing
TEST(ExprMutationAnalyzerTest, Move) {
More information about the cfe-commits
mailing list