EricWF created this revision.
EricWF added reviewers: fowles, shuaiwang.
EricWF added a project: clang-tools-extra.
Herald added subscribers: mgehre, xazax.hun.
Currently the range-for-copy incorrectly suggests changing a
by-value loop var to a reference to avoid copies even when:
(1) A converting constructor was used. Or,
(2) The argument to the copy constructor is not an lvalue.
For example:
for (const std::string sv : std::vector<const char*>{}) {
// warning:
// the loop variable's type is not a reference type;
// this creates a copy in each iteration;
// consider making this a reference
}
In these cases we can't actually avoid the copy, and reference
binding + lifetime extending a temporary is not a "fix" we should
be suggesting.
Admittedly, cases like the example should be fixed by the clang-tidy
user, but at minimum we need to be clearer about when a copy is made
and when a user defined conversion occurs (and that conversion may
be semantically important).
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D78223
Files:
clang-tools-extra/clang-tidy/performance/ForRangeCopyCheck.cpp
clang-tools-extra/test/clang-tidy/checkers/performance-for-range-copy.cpp
Index: clang-tools-extra/test/clang-tidy/checkers/performance-for-range-copy.cpp
===================================================================
--- clang-tools-extra/test/clang-tidy/checkers/performance-for-range-copy.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/performance-for-range-copy.cpp
@@ -1,4 +1,5 @@
// RUN: %check_clang_tidy %s performance-for-range-copy %t -- --
-fno-delayed-template-parsing
+// RUN: %check_clang_tidy %s -std=c++17 performance-for-range-copy %t -- --
-fno-delayed-template-parsing
namespace std {
@@ -46,6 +47,7 @@
S &operator=(const S &);
};
+
struct Convertible {
operator S() const {
return S();
@@ -61,12 +63,16 @@
Convertible C[0];
for (const S &S1 : C) {
}
+ for (const S S2 : C) {
+ }
}
void negativeImplicitConstructorConversion() {
ConstructorConvertible C[0];
for (const S &S1 : C) {
}
+ for (const S S2 : C) {
+ }
}
template <typename T>
Index: clang-tools-extra/clang-tidy/performance/ForRangeCopyCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/performance/ForRangeCopyCheck.cpp
+++ clang-tools-extra/clang-tidy/performance/ForRangeCopyCheck.cpp
@@ -60,6 +60,18 @@
handleCopyIsOnlyConstReferenced(*Var, *ForRange, *Result.Context);
}
+// Returns true iff the specified variable is initialized via copy construction
+// and the argument to the copy constructor is an lvalue.
+//
+// Note: We don't want to suggest reference binding + lifetime extending a
+// loop index because it's less clear than copying the temporary.
+static bool isCopyConstructedFromLValueArg(const VarDecl &LoopVar, ASTContext&
Ctx) {
+ const auto *Init = dyn_cast_or_null<CXXConstructExpr>(LoopVar.getInit());
+ if (!Init || !Init->getConstructor()->isCopyConstructor() ||
Init->getNumArgs() < 1)
+ return false;
+ return Init->getArg(0)->isLValue();
+}
+
bool ForRangeCopyCheck::handleConstValueCopy(const VarDecl &LoopVar,
ASTContext &Context) {
if (WarnOnAllAutoCopies) {
@@ -73,6 +85,8 @@
utils::type_traits::isExpensiveToCopy(LoopVar.getType(), Context);
if (!Expensive || !*Expensive)
return false;
+ if (!isCopyConstructedFromLValueArg(LoopVar, Context))
+ return false;
auto Diagnostic =
diag(LoopVar.getLocation(),
"the loop variable's type is not a reference type; this creates a "
@@ -93,6 +107,8 @@
utils::type_traits::isExpensiveToCopy(LoopVar.getType(), Context);
if (LoopVar.getType().isConstQualified() || !Expensive || !*Expensive)
return false;
+ if (!isCopyConstructedFromLValueArg(LoopVar, Context))
+ return false;
// We omit the case where the loop variable is not used in the loop body.
E.g.
//
// for (auto _ : benchmark_state) {
Index: clang-tools-extra/test/clang-tidy/checkers/performance-for-range-copy.cpp
===================================================================
--- clang-tools-extra/test/clang-tidy/checkers/performance-for-range-copy.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/performance-for-range-copy.cpp
@@ -1,4 +1,5 @@
// RUN: %check_clang_tidy %s performance-for-range-copy %t -- -- -fno-delayed-template-parsing
+// RUN: %check_clang_tidy %s -std=c++17 performance-for-range-copy %t -- -- -fno-delayed-template-parsing
namespace std {
@@ -46,6 +47,7 @@
S &operator=(const S &);
};
+
struct Convertible {
operator S() const {
return S();
@@ -61,12 +63,16 @@
Convertible C[0];
for (const S &S1 : C) {
}
+ for (const S S2 : C) {
+ }
}
void negativeImplicitConstructorConversion() {
ConstructorConvertible C[0];
for (const S &S1 : C) {
}
+ for (const S S2 : C) {
+ }
}
template <typename T>
Index: clang-tools-extra/clang-tidy/performance/ForRangeCopyCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/performance/ForRangeCopyCheck.cpp
+++ clang-tools-extra/clang-tidy/performance/ForRangeCopyCheck.cpp
@@ -60,6 +60,18 @@
handleCopyIsOnlyConstReferenced(*Var, *ForRange, *Result.Context);
}
+// Returns true iff the specified variable is initialized via copy construction
+// and the argument to the copy constructor is an lvalue.
+//
+// Note: We don't want to suggest reference binding + lifetime extending a
+// loop index because it's less clear than copying the temporary.
+static bool isCopyConstructedFromLValueArg(const VarDecl &LoopVar, ASTContext& Ctx) {
+ const auto *Init = dyn_cast_or_null<CXXConstructExpr>(LoopVar.getInit());
+ if (!Init || !Init->getConstructor()->isCopyConstructor() || Init->getNumArgs() < 1)
+ return false;
+ return Init->getArg(0)->isLValue();
+}
+
bool ForRangeCopyCheck::handleConstValueCopy(const VarDecl &LoopVar,
ASTContext &Context) {
if (WarnOnAllAutoCopies) {
@@ -73,6 +85,8 @@
utils::type_traits::isExpensiveToCopy(LoopVar.getType(), Context);
if (!Expensive || !*Expensive)
return false;
+ if (!isCopyConstructedFromLValueArg(LoopVar, Context))
+ return false;
auto Diagnostic =
diag(LoopVar.getLocation(),
"the loop variable's type is not a reference type; this creates a "
@@ -93,6 +107,8 @@
utils::type_traits::isExpensiveToCopy(LoopVar.getType(), Context);
if (LoopVar.getType().isConstQualified() || !Expensive || !*Expensive)
return false;
+ if (!isCopyConstructedFromLValueArg(LoopVar, Context))
+ return false;
// We omit the case where the loop variable is not used in the loop body. E.g.
//
// for (auto _ : benchmark_state) {
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
