malcolm.parsons created this revision.
malcolm.parsons added reviewers: aaron.ballman, flx, alexfh.
malcolm.parsons added a subscriber: cfe-commits.
Herald added a subscriber: JDevlieghere.
modernize-pass-by-value doesn't warn about value parameters that
cannot be moved, so performance-unnecessary-value-param should.
https://reviews.llvm.org/D28022
Files:
clang-tidy/performance/UnnecessaryValueParamCheck.cpp
clang-tidy/utils/DeclRefExprUtils.cpp
clang-tidy/utils/DeclRefExprUtils.h
test/clang-tidy/performance-unnecessary-value-param-delayed.cpp
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
@@ -82,6 +82,7 @@
struct PositiveConstValueConstructor {
PositiveConstValueConstructor(const ExpensiveToCopyType ConstCopy) {}
// CHECK-MESSAGES: [[@LINE-1]]:59: warning: the const qualified parameter 'ConstCopy'
+ // CHECK-FIXES: PositiveConstValueConstructor(const ExpensiveToCopyType& ConstCopy) {}
};
template <typename T> void templateWithNonTemplatizedParameter(const ExpensiveToCopyType S, T V) {
@@ -143,8 +144,29 @@
Obj.nonConstMethod();
}
-struct NegativeValueCopyConstructor {
- NegativeValueCopyConstructor(ExpensiveToCopyType Copy) {}
+struct PositiveValueUnusedConstructor {
+ PositiveValueUnusedConstructor(ExpensiveToCopyType Copy) {}
+ // CHECK-MESSAGES: [[@LINE-1]]:54: warning: the parameter 'Copy'
+ // CHECK-FIXES: PositiveValueUnusedConstructor(const ExpensiveToCopyType& Copy) {}
+};
+
+struct PositiveValueCopiedConstructor {
+ PositiveValueCopiedConstructor(ExpensiveToCopyType Copy) : Field(Copy) {}
+ // CHECK-MESSAGES: [[@LINE-1]]:54: warning: the parameter 'Copy'
+ // CHECK-FIXES: PositiveValueCopiedConstructor(const ExpensiveToCopyType& Copy) : Field(Copy) {}
+ ExpensiveToCopyType Field;
+};
+
+struct PositiveValueMovableConstructor {
+ PositiveValueMovableConstructor(ExpensiveMovableType Copy) : Field(Copy) {}
+ // CHECK-MESSAGES: [[@LINE-1]]:70: warning: parameter 'Copy'
+ // CHECK-FIXES: PositiveValueMovableConstructor(ExpensiveMovableType Copy) : Field(std::move(Copy)) {}
+ ExpensiveMovableType Field;
+};
+
+struct NegativeValueMovedConstructor {
+ NegativeValueMovedConstructor(ExpensiveMovableType Copy) : Field(static_cast<ExpensiveMovableType &&>(Copy)) {}
+ ExpensiveMovableType Field;
};
template <typename T>
Index: test/clang-tidy/performance-unnecessary-value-param-delayed.cpp
===================================================================
--- test/clang-tidy/performance-unnecessary-value-param-delayed.cpp
+++ test/clang-tidy/performance-unnecessary-value-param-delayed.cpp
@@ -64,6 +64,7 @@
struct PositiveConstValueConstructor {
PositiveConstValueConstructor(const ExpensiveToCopyType ConstCopy) {}
// CHECK-MESSAGES: [[@LINE-1]]:59: warning: the const qualified parameter 'ConstCopy'
+ // CHECK-FIXES: PositiveConstValueConstructor(const ExpensiveToCopyType& ConstCopy) {}
};
template <typename T> void templateWithNonTemplatizedParameter(const ExpensiveToCopyType S, T V) {
@@ -125,8 +126,17 @@
Obj.nonConstMethod();
}
-struct NegativeValueCopyConstructor {
- NegativeValueCopyConstructor(ExpensiveToCopyType Copy) {}
+struct PositiveValueUnusedConstructor {
+ PositiveValueUnusedConstructor(ExpensiveToCopyType Copy) {}
+ // CHECK-MESSAGES: [[@LINE-1]]:54: warning: the parameter 'Copy'
+ // CHECK-FIXES: PositiveValueUnusedConstructor(const ExpensiveToCopyType& Copy) {}
+};
+
+struct PositiveValueCopiedConstructor {
+ PositiveValueCopiedConstructor(ExpensiveToCopyType Copy) : Field(Copy) {}
+ // CHECK-MESSAGES: [[@LINE-1]]:54: warning: the parameter 'Copy'
+ // CHECK-FIXES: PositiveValueCopiedConstructor(const ExpensiveToCopyType& Copy) : Field(Copy) {}
+ ExpensiveToCopyType Field;
};
template <typename T>
Index: clang-tidy/utils/DeclRefExprUtils.h
===================================================================
--- clang-tidy/utils/DeclRefExprUtils.h
+++ clang-tidy/utils/DeclRefExprUtils.h
@@ -32,19 +32,29 @@
llvm::SmallPtrSet<const DeclRefExpr *, 16>
allDeclRefExprs(const VarDecl &VarDecl, const Stmt &Stmt, ASTContext &Context);
+// Returns set of all DeclRefExprs to VarDecl in Decl.
+llvm::SmallPtrSet<const DeclRefExpr *, 16>
+allDeclRefExprs(const VarDecl &VarDecl, const Decl &Decl, ASTContext &Context);
+
// Returns set of all DeclRefExprs where VarDecl is guaranteed to be accessed in
// a const fashion.
llvm::SmallPtrSet<const DeclRefExpr *, 16>
constReferenceDeclRefExprs(const VarDecl &VarDecl, const Stmt &Stmt,
ASTContext &Context);
+// Returns set of all DeclRefExprs where VarDecl is guaranteed to be accessed in
+// a const fashion.
+llvm::SmallPtrSet<const DeclRefExpr *, 16>
+constReferenceDeclRefExprs(const VarDecl &VarDecl, const Decl &Decl,
+ ASTContext &Context);
+
// Returns true if DeclRefExpr is the argument of a copy-constructor call expr.
-bool isCopyConstructorArgument(const DeclRefExpr &DeclRef, const Stmt &Stmt,
+bool isCopyConstructorArgument(const DeclRefExpr &DeclRef, const Decl &Decl,
ASTContext &Context);
// Returns true if DeclRefExpr is the argument of a copy-assignment operator
// call expr.
-bool isCopyAssignmentArgument(const DeclRefExpr &DeclRef, const Stmt &Stmt,
+bool isCopyAssignmentArgument(const DeclRefExpr &DeclRef, const Decl &Decl,
ASTContext &Context);
} // namespace decl_ref_expr
Index: clang-tidy/utils/DeclRefExprUtils.cpp
===================================================================
--- clang-tidy/utils/DeclRefExprUtils.cpp
+++ clang-tidy/utils/DeclRefExprUtils.cpp
@@ -71,6 +71,40 @@
return DeclRefs;
}
+// Finds all DeclRefExprs where a const method is called on VarDecl or VarDecl
+// is the a const reference or value argument to a CallExpr or CXXConstructExpr.
+SmallPtrSet<const DeclRefExpr *, 16>
+constReferenceDeclRefExprs(const VarDecl &VarDecl, const Decl &Decl,
+ ASTContext &Context) {
+ auto DeclRefToVar =
+ declRefExpr(to(varDecl(equalsNode(&VarDecl)))).bind("declRef");
+ auto ConstMethodCallee = callee(cxxMethodDecl(isConst()));
+ // Match method call expressions where the variable is referenced as the this
+ // implicit object argument and opertor call expression for member operators
+ // where the variable is the 0-th argument.
+ auto Matches =
+ match(decl(forEachDescendant(expr(
+ anyOf(cxxMemberCallExpr(ConstMethodCallee, on(DeclRefToVar)),
+ cxxOperatorCallExpr(ConstMethodCallee,
+ hasArgument(0, DeclRefToVar)))))),
+ Decl, Context);
+ SmallPtrSet<const DeclRefExpr *, 16> DeclRefs;
+ extractNodesByIdTo(Matches, "declRef", DeclRefs);
+ auto ConstReferenceOrValue =
+ qualType(anyOf(referenceType(pointee(qualType(isConstQualified()))),
+ unless(anyOf(referenceType(), pointerType()))));
+ auto UsedAsConstRefOrValueArg = forEachArgumentWithParam(
+ DeclRefToVar, parmVarDecl(hasType(ConstReferenceOrValue)));
+ Matches = match(decl(forEachDescendant(callExpr(UsedAsConstRefOrValueArg))),
+ Decl, Context);
+ extractNodesByIdTo(Matches, "declRef", DeclRefs);
+ Matches =
+ match(decl(forEachDescendant(cxxConstructExpr(UsedAsConstRefOrValueArg))),
+ Decl, Context);
+ extractNodesByIdTo(Matches, "declRef", DeclRefs);
+ return DeclRefs;
+}
+
bool isOnlyUsedAsConst(const VarDecl &Var, const Stmt &Stmt,
ASTContext &Context) {
// Collect all DeclRefExprs to the loop variable and all CallExprs and
@@ -93,30 +127,41 @@
return DeclRefs;
}
-bool isCopyConstructorArgument(const DeclRefExpr &DeclRef, const Stmt &Stmt,
+SmallPtrSet<const DeclRefExpr *, 16>
+allDeclRefExprs(const VarDecl &VarDecl, const Decl &Decl, ASTContext &Context) {
+ auto Matches = match(
+ decl(forEachDescendant(
+ declRefExpr(to(varDecl(equalsNode(&VarDecl)))).bind("declRef"))),
+ Decl, Context);
+ SmallPtrSet<const DeclRefExpr *, 16> DeclRefs;
+ extractNodesByIdTo(Matches, "declRef", DeclRefs);
+ return DeclRefs;
+}
+
+bool isCopyConstructorArgument(const DeclRefExpr &DeclRef, const Decl &Decl,
ASTContext &Context) {
auto UsedAsConstRefArg = forEachArgumentWithParam(
declRefExpr(equalsNode(&DeclRef)),
parmVarDecl(hasType(matchers::isReferenceToConst())));
auto Matches = match(
- stmt(hasDescendant(
+ decl(hasDescendant(
cxxConstructExpr(UsedAsConstRefArg, hasDeclaration(cxxConstructorDecl(
isCopyConstructor())))
.bind("constructExpr"))),
- Stmt, Context);
+ Decl, Context);
return !Matches.empty();
}
-bool isCopyAssignmentArgument(const DeclRefExpr &DeclRef, const Stmt &Stmt,
+bool isCopyAssignmentArgument(const DeclRefExpr &DeclRef, const Decl &Decl,
ASTContext &Context) {
auto UsedAsConstRefArg = forEachArgumentWithParam(
declRefExpr(equalsNode(&DeclRef)),
parmVarDecl(hasType(matchers::isReferenceToConst())));
auto Matches = match(
- stmt(hasDescendant(
+ decl(hasDescendant(
cxxOperatorCallExpr(UsedAsConstRefArg, hasOverloadedOperatorName("="))
.bind("operatorCallExpr"))),
- Stmt, Context);
+ Decl, Context);
return !Matches.empty();
}
Index: clang-tidy/performance/UnnecessaryValueParamCheck.cpp
===================================================================
--- clang-tidy/performance/UnnecessaryValueParamCheck.cpp
+++ clang-tidy/performance/UnnecessaryValueParamCheck.cpp
@@ -47,14 +47,14 @@
return !Matches.empty();
}
-bool hasLoopStmtAncestor(const DeclRefExpr &DeclRef, const Stmt &Stmt,
+bool hasLoopStmtAncestor(const DeclRefExpr &DeclRef, const Decl &Decl,
ASTContext &Context) {
auto Matches =
- match(findAll(declRefExpr(
+ match(decl(forEachDescendant(declRefExpr(
equalsNode(&DeclRef),
unless(hasAncestor(stmt(anyOf(forStmt(), cxxForRangeStmt(),
- whileStmt(), doStmt())))))),
- Stmt, Context);
+ whileStmt(), doStmt()))))))),
+ Decl, Context);
return Matches.empty();
}
@@ -90,21 +90,16 @@
Param->getType().getCanonicalType().isConstQualified();
// Skip declarations delayed by late template parsing without a body.
- if (!Function->getBody())
- return;
-
- // Do not trigger on non-const value parameters when:
- // 1. they are in a constructor definition since they can likely trigger
- // modernize-pass-by-value which will suggest to move the argument.
- if (!IsConstQualified && (llvm::isa<CXXConstructorDecl>(Function) ||
- !Function->doesThisDeclarationHaveABody()))
+ if (!Function->hasBody())
return;
auto AllDeclRefExprs = utils::decl_ref_expr::allDeclRefExprs(
- *Param, *Function->getBody(), *Result.Context);
+ *Param, *Function->getDefinition(), *Result.Context);
auto ConstDeclRefExprs = utils::decl_ref_expr::constReferenceDeclRefExprs(
- *Param, *Function->getBody(), *Result.Context);
- // 2. they are not only used as const.
+ *Param, *Function->getDefinition(), *Result.Context);
+
+ // Do not trigger on non-const value parameters when they are not only used as
+ // const.
if (!isSubset(AllDeclRefExprs, ConstDeclRefExprs))
return;
@@ -116,15 +111,15 @@
if (!IsConstQualified) {
auto CanonicalType = Param->getType().getCanonicalType();
if (AllDeclRefExprs.size() == 1 &&
- !hasLoopStmtAncestor(**AllDeclRefExprs.begin(), *Function->getBody(),
- *Result.Context) &&
+ !hasLoopStmtAncestor(**AllDeclRefExprs.begin(),
+ *Function->getDefinition(), *Result.Context) &&
((utils::type_traits::hasNonTrivialMoveConstructor(CanonicalType) &&
utils::decl_ref_expr::isCopyConstructorArgument(
- **AllDeclRefExprs.begin(), *Function->getBody(),
+ **AllDeclRefExprs.begin(), *Function->getDefinition(),
*Result.Context)) ||
(utils::type_traits::hasNonTrivialMoveAssignment(CanonicalType) &&
utils::decl_ref_expr::isCopyAssignmentArgument(
- **AllDeclRefExprs.begin(), *Function->getBody(),
+ **AllDeclRefExprs.begin(), *Function->getDefinition(),
*Result.Context)))) {
handleMoveFix(*Param, **AllDeclRefExprs.begin(), *Result.Context);
return;
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
