flx created this revision.
flx added reviewers: alexfh, sbenza, aaron.ballman.
flx added a subscriber: cfe-commits.
flx set the repository for this revision to rL LLVM.
flx added a project: clang-tools-extra.
Herald added a subscriber: JDevlieghere.
This fixes a bug where the performance-unnecessary-value-param check suggests a
fix to move the parameter inside of a loop which could be invoked multiple
times.
Repository:
rL LLVM
https://reviews.llvm.org/D27187
Files:
clang-tidy/performance/UnnecessaryValueParamCheck.cpp
clang-tidy/utils/DeclRefExprUtils.cpp
clang-tidy/utils/DeclRefExprUtils.h
test/clang-tidy/performance-unnecessary-value-param.cpp
Index: test/clang-tidy/performance-unnecessary-value-param.cpp
===================================================================
--- test/clang-tidy/performance-unnecessary-value-param.cpp
+++ test/clang-tidy/performance-unnecessary-value-param.cpp
@@ -225,6 +225,15 @@
// CHECK-FIXES: F = std::move(E);
}
+// The argument could be moved but is not since copy statement is inside a
loop.
+void PositiveNoMoveInsideLoop(ExpensiveMovableType E) {
+ // CHECK-MESSAGES: [[@LINE-1]]:52: warning: the parameter 'E' is copied
+ // CHECK-FIXES: void PositiveNoMoveInsideLoop(const ExpensiveMovableType& E)
{
+ for (;;) {
+ auto F = E;
+ }
+}
+
void PositiveConstRefNotMoveConstructible(ExpensiveToCopyType T) {
// CHECK-MESSAGES: [[@LINE-1]]:63: warning: the parameter 'T' is copied
// CHECK-FIXES: void PositiveConstRefNotMoveConstructible(const
ExpensiveToCopyType& T) {
Index: clang-tidy/utils/DeclRefExprUtils.h
===================================================================
--- clang-tidy/utils/DeclRefExprUtils.h
+++ clang-tidy/utils/DeclRefExprUtils.h
@@ -47,6 +47,11 @@
bool isCopyAssignmentArgument(const DeclRefExpr &DeclRef, const Stmt &Stmt,
ASTContext &Context);
+// Returns true if DeclRefExpr has a loop statement (ForStmt, CXXForRangeStmt,
+// WhileStmt, DoStmt) as ancestor.
+bool hasLoopStmtAncestor(const DeclRefExpr &DeclRef, const Stmt &Stmt,
+ ASTContext &Context);
+
} // namespace decl_ref_expr
} // namespace utils
} // namespace tidy
Index: clang-tidy/utils/DeclRefExprUtils.cpp
===================================================================
--- clang-tidy/utils/DeclRefExprUtils.cpp
+++ clang-tidy/utils/DeclRefExprUtils.cpp
@@ -120,6 +120,17 @@
return !Matches.empty();
}
+bool hasLoopStmtAncestor(const DeclRefExpr &DeclRef, const Stmt &Stmt,
+ ASTContext &Context) {
+ auto Matches =
+ match(findAll(declRefExpr(equalsNode(&DeclRef),
+ unless(hasAncestor(stmt(anyOf(
+ forStmt(), cxxForRangeStmt(), whileStmt(),
+ doStmt()))))).bind("declRef")),
+ Stmt, Context);
+ return Matches.empty();
+}
+
} // namespace decl_ref_expr
} // namespace utils
} // namespace tidy
Index: clang-tidy/performance/UnnecessaryValueParamCheck.cpp
===================================================================
--- clang-tidy/performance/UnnecessaryValueParamCheck.cpp
+++ clang-tidy/performance/UnnecessaryValueParamCheck.cpp
@@ -104,6 +104,8 @@
if (!IsConstQualified) {
auto CanonicalType = Param->getType().getCanonicalType();
if (AllDeclRefExprs.size() == 1 &&
+ !utils::decl_ref_expr::hasLoopStmtAncestor(
+ **AllDeclRefExprs.begin(), *Function->getBody(), *Result.Context)
&&
((utils::type_traits::hasNonTrivialMoveConstructor(CanonicalType) &&
utils::decl_ref_expr::isCopyConstructorArgument(
**AllDeclRefExprs.begin(), *Function->getBody(),
Index: test/clang-tidy/performance-unnecessary-value-param.cpp
===================================================================
--- test/clang-tidy/performance-unnecessary-value-param.cpp
+++ test/clang-tidy/performance-unnecessary-value-param.cpp
@@ -225,6 +225,15 @@
// CHECK-FIXES: F = std::move(E);
}
+// The argument could be moved but is not since copy statement is inside a loop.
+void PositiveNoMoveInsideLoop(ExpensiveMovableType E) {
+ // CHECK-MESSAGES: [[@LINE-1]]:52: warning: the parameter 'E' is copied
+ // CHECK-FIXES: void PositiveNoMoveInsideLoop(const ExpensiveMovableType& E) {
+ for (;;) {
+ auto F = E;
+ }
+}
+
void PositiveConstRefNotMoveConstructible(ExpensiveToCopyType T) {
// CHECK-MESSAGES: [[@LINE-1]]:63: warning: the parameter 'T' is copied
// CHECK-FIXES: void PositiveConstRefNotMoveConstructible(const ExpensiveToCopyType& T) {
Index: clang-tidy/utils/DeclRefExprUtils.h
===================================================================
--- clang-tidy/utils/DeclRefExprUtils.h
+++ clang-tidy/utils/DeclRefExprUtils.h
@@ -47,6 +47,11 @@
bool isCopyAssignmentArgument(const DeclRefExpr &DeclRef, const Stmt &Stmt,
ASTContext &Context);
+// Returns true if DeclRefExpr has a loop statement (ForStmt, CXXForRangeStmt,
+// WhileStmt, DoStmt) as ancestor.
+bool hasLoopStmtAncestor(const DeclRefExpr &DeclRef, const Stmt &Stmt,
+ ASTContext &Context);
+
} // namespace decl_ref_expr
} // namespace utils
} // namespace tidy
Index: clang-tidy/utils/DeclRefExprUtils.cpp
===================================================================
--- clang-tidy/utils/DeclRefExprUtils.cpp
+++ clang-tidy/utils/DeclRefExprUtils.cpp
@@ -120,6 +120,17 @@
return !Matches.empty();
}
+bool hasLoopStmtAncestor(const DeclRefExpr &DeclRef, const Stmt &Stmt,
+ ASTContext &Context) {
+ auto Matches =
+ match(findAll(declRefExpr(equalsNode(&DeclRef),
+ unless(hasAncestor(stmt(anyOf(
+ forStmt(), cxxForRangeStmt(), whileStmt(),
+ doStmt()))))).bind("declRef")),
+ Stmt, Context);
+ return Matches.empty();
+}
+
} // namespace decl_ref_expr
} // namespace utils
} // namespace tidy
Index: clang-tidy/performance/UnnecessaryValueParamCheck.cpp
===================================================================
--- clang-tidy/performance/UnnecessaryValueParamCheck.cpp
+++ clang-tidy/performance/UnnecessaryValueParamCheck.cpp
@@ -104,6 +104,8 @@
if (!IsConstQualified) {
auto CanonicalType = Param->getType().getCanonicalType();
if (AllDeclRefExprs.size() == 1 &&
+ !utils::decl_ref_expr::hasLoopStmtAncestor(
+ **AllDeclRefExprs.begin(), *Function->getBody(), *Result.Context) &&
((utils::type_traits::hasNonTrivialMoveConstructor(CanonicalType) &&
utils::decl_ref_expr::isCopyConstructorArgument(
**AllDeclRefExprs.begin(), *Function->getBody(),
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
