[clang] [clang-tools-extra] [clang-tidy] Handle C++ structured bindings in `performance-for-range-copy` (PR #77105)
@@ -234,6 +239,22 @@ void positiveOnlyAccessedFieldAsConst() {
}
}
+void positiveOnlyAccessedFieldAsConstBinding() {
legrosbuffle wrote:
Done.
https://github.com/llvm/llvm-project/pull/77105
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [clang-tidy] Handle C++ structured bindings in `performance-for-range-copy` (PR #77105)
@@ -234,6 +239,22 @@ void positiveOnlyAccessedFieldAsConst() {
}
}
+void positiveOnlyAccessedFieldAsConstBinding() {
+ for (auto [X, Y] : View>()) {
+// CHECK-MESSAGES: [[@LINE-1]]:13: warning: loop variable is copied but
+// CHECK-FIXES: for (const auto& [X, Y] : View>()) {
+use(X);
+use(Y);
+ }
+}
+
+void negativeOnlyAccessedFieldAsConstBinding() {
legrosbuffle wrote:
Done.
https://github.com/llvm/llvm-project/pull/77105
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [clang-tidy] Handle C++ structured bindings in `performance-for-range-copy` (PR #77105)
@@ -234,6 +239,22 @@ void positiveOnlyAccessedFieldAsConst() {
}
}
+void positiveOnlyAccessedFieldAsConstBinding() {
fberger wrote:
Why `AccessedField` here, it seems to be `JustUsedAsConst` as in other test
names?
https://github.com/llvm/llvm-project/pull/77105
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [clang-tidy] Handle C++ structured bindings in `performance-for-range-copy` (PR #77105)
https://github.com/fberger approved this pull request. https://github.com/llvm/llvm-project/pull/77105 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [clang-tidy] Handle C++ structured bindings in `performance-for-range-copy` (PR #77105)
https://github.com/fberger edited https://github.com/llvm/llvm-project/pull/77105 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [clang-tidy] Handle C++ structured bindings in `performance-for-range-copy` (PR #77105)
llvmbot wrote:
@llvm/pr-subscribers-clang-tidy
Author: Clement Courbet (legrosbuffle)
Changes
Right now we are not triggering on:
```
for (auto [x, y] : container) {
// const-only access
}
```
---
Full diff: https://github.com/llvm/llvm-project/pull/77105.diff
5 Files Affected:
- (modified) clang-tools-extra/clang-tidy/performance/ForRangeCopyCheck.cpp
(+10-3)
- (modified) clang-tools-extra/docs/ReleaseNotes.rst (+4)
- (modified)
clang-tools-extra/test/clang-tidy/checkers/performance/for-range-copy.cpp
(+26-5)
- (modified) clang/lib/Analysis/ExprMutationAnalyzer.cpp (+10-3)
- (modified) clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp (+20)
``diff
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
,
return true;
}
+static bool isReferenced(const VarDecl , const Stmt ,
+ ASTContext ) {
+ const auto IsLoopVar = varDecl(equalsNode());
+ return !match(stmt(hasDescendant(declRefExpr(to(valueDecl(anyOf(
+IsLoopVar, bindingDecl(forDecomposition(IsLoopVar,
+Stmt, Context)
+ .empty();
+}
+
bool ForRangeCopyCheck::handleCopyIsOnlyConstReferenced(
const VarDecl , const CXXForRangeStmt ,
ASTContext ) {
@@ -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()
&&
- !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
` check to properly escape
single quotes.
+- Improved :doc:`performance-for-range-copy
+ ` check to handle cases where
+ the loop variable is a structured binding.
+
- Improved :doc:`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 =(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>()) {}
+ for (const auto [X, Y] : View>()) {}
+ // CHECK-MESSAGES: [[@LINE-1]]:19: warning: the loop variable's type is
+ // CHECK-FIXES: {{^}} for (const auto& [X, Y] : View>()) {}
+
for (const T T2 : View>()) {}
// CHECK-MESSAGES: [[@LINE-1]]:16: warning: the loop variable's type is
{{.*}}
// CHECK-FIXES: {{^}} for (const T& T2 : View>()) {}
@@ -123,11 +132,6 @@ struct Mutable {
~Mutable() {}
};
-struct Point {
- ~Point() {}
- int x, y;
-};
-
Mutable& operator<<(Mutable , 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 );
+void mutate(int &);
void onceConstOnceMutated(const Mutable , Mutable );
void negativeVariableIsMutated() {
@@ -234,6 +239,22 @@ void positiveOnlyAccessedFieldAsConst() {
}
}
+void positiveOnlyAccessedFieldAsConstBinding() {
+ for (auto [X, Y] : View>()) {
+// CHECK-MESSAGES: [[@LINE-1]]:13: warning: loop variable is copied but
+// CHECK-FIXES: for (const auto& [X, Y] : View>()) {
+use(X);
+use(Y);
+ }
+}
+
+void negativeOnlyAccessedFieldAsConstBinding() {
+ for (auto [X, Y] : View>()) {
+use(X);
+mutate(Y);
+ }
+}
+
void positiveOnlyUsedInCopyConstructor() {
for (auto A : View>()) {
// CHECK-MESSAGES: [[@LINE-1]]:13: warning: loop variable is copied but
only used as const reference; consider making it a const reference
[clang] [clang-tools-extra] [clang-tidy] Handle C++ structured bindings in `performance-for-range-copy` (PR #77105)
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
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
,
return true;
}
+static bool isReferenced(const VarDecl , const Stmt ,
+ ASTContext ) {
+ const auto IsLoopVar = varDecl(equalsNode());
+ return !match(stmt(hasDescendant(declRefExpr(to(valueDecl(anyOf(
+IsLoopVar, bindingDecl(forDecomposition(IsLoopVar,
+Stmt, Context)
+ .empty();
+}
+
bool ForRangeCopyCheck::handleCopyIsOnlyConstReferenced(
const VarDecl , const CXXForRangeStmt ,
ASTContext ) {
@@ -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()
&&
- !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
` check to properly escape
single quotes.
+- Improved :doc:`performance-for-range-copy
+ ` check to handle cases where
+ the loop variable is a structured binding.
+
- Improved :doc:`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 =(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>()) {}
+ for (const auto [X, Y] : View>()) {}
+ // CHECK-MESSAGES: [[@LINE-1]]:19: warning: the loop variable's type is
+ // CHECK-FIXES: {{^}} for (const auto& [X, Y] : View>()) {}
+
for (const T T2 : View>()) {}
// CHECK-MESSAGES: [[@LINE-1]]:16: warning: the loop variable's type is
{{.*}}
// CHECK-FIXES: {{^}} for (const T& T2 : View>()) {}
@@ -123,11 +132,6 @@ struct Mutable {
~Mutable() {}
};
-struct Point {
- ~Point() {}
- int x, y;
-};
-
Mutable& operator<<(Mutable , 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 );
+void mutate(int &);
void onceConstOnceMutated(const Mutable , Mutable );
void negativeVariableIsMutated() {
@@ -234,6 +239,22 @@ void positiveOnlyAccessedFieldAsConst() {
}
}
+void positiveOnlyAccessedFieldAsConstBinding() {
+ for (auto [X, Y] : View>()) {
+// CHECK-MESSAGES: [[@LINE-1]]:13: warning: loop variable is copied but
+// CHECK-FIXES: for (const auto& [X, Y] : View>()) {
+use(X);
+use(Y);
+ }
+}
+
+void negativeOnlyAccessedFieldAsConstBinding() {
+ for (auto [X, Y] : View>()) {
+use(X);
+mutate(Y);
+ }
+}
+
void
