maxjhandsome
/
llvm-project
forked from OSchip/llvm-project
1
0
Fork 0
Code Issues Pull Requests Packages Releases Wiki Activity

[clang-tidy] Handle constructors in performance-unnecessary-value-param 

Summary:
modernize-pass-by-value doesn't warn about value parameters that
cannot be moved, so performance-unnecessary-value-param should.

Reviewers: aaron.ballman, flx, alexfh

Subscribers: JDevlieghere, cfe-commits

Differential Revision: https://reviews.llvm.org/D28022

llvm-svn: 290883
This commit is contained in:
Malcolm Parsons 2017-01-03 12:10:44 +00:00
parent 009978b4d7
commit cfa7d3748e
5 changed files with 123 additions and 46 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

45
clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
View File

@ -47,14 +47,14 @@ bool isReferencedOutsideOfCallExpr(const FunctionDecl &Function,
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();
}
@ -72,7 +72,7 @@ void UnnecessaryValueParamCheck::registerMatchers(MatchFinder *Finder) {
unless(referenceType())))),
decl().bind("param"));
Finder->addMatcher(
functionDecl(isDefinition(),
functionDecl(hasBody(stmt()), isDefinition(),
unless(cxxMethodDecl(anyOf(isOverride(), isFinal()))),
unless(isInstantiated()),
has(typeLoc(forEach(ExpensiveValueParamDecl))),
@ -89,22 +89,13 @@ void UnnecessaryValueParamCheck::check(const MatchFinder::MatchResult &Result) {
bool IsConstQualified =
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()))
return;
auto AllDeclRefExprs = utils::decl_ref_expr::allDeclRefExprs(
*Param, *Function->getBody(), *Result.Context);
*Param, *Function, *Result.Context);
auto ConstDeclRefExprs = utils::decl_ref_expr::constReferenceDeclRefExprs(
*Param, *Function->getBody(), *Result.Context);
// 2. they are not only used as const.
*Param, *Function, *Result.Context);
// Do not trigger on non-const value parameters when they are not only used as
// const.
if (!isSubset(AllDeclRefExprs, ConstDeclRefExprs))
return;
@ -113,20 +104,18 @@ void UnnecessaryValueParamCheck::check(const MatchFinder::MatchResult &Result) {
// move assignment operator and is only referenced once when copy-assigned.
// In this case wrap DeclRefExpr with std::move() to avoid the unnecessary
// copy.
if (!IsConstQualified) {
if (!IsConstQualified && AllDeclRefExprs.size() == 1) {
auto CanonicalType = Param->getType().getCanonicalType();
if (AllDeclRefExprs.size() == 1 &&
!hasLoopStmtAncestor(**AllDeclRefExprs.begin(), *Function->getBody(),
*Result.Context) &&
const auto &DeclRefExpr = **AllDeclRefExprs.begin();
if (!hasLoopStmtAncestor(DeclRefExpr, *Function, *Result.Context) &&
((utils::type_traits::hasNonTrivialMoveConstructor(CanonicalType) &&
utils::decl_ref_expr::isCopyConstructorArgument(
**AllDeclRefExprs.begin(), *Function->getBody(),
*Result.Context)) ||
DeclRefExpr, *Function, *Result.Context)) ||
(utils::type_traits::hasNonTrivialMoveAssignment(CanonicalType) &&
utils::decl_ref_expr::isCopyAssignmentArgument(
**AllDeclRefExprs.begin(), *Function->getBody(),
*Result.Context)))) {
handleMoveFix(*Param, **AllDeclRefExprs.begin(), *Result.Context);
DeclRefExpr, *Function, *Result.Context)))) {
handleMoveFix(*Param, DeclRefExpr, *Result.Context);
return;
}
}

57
clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
View File

@ -71,6 +71,40 @@ constReferenceDeclRefExprs(const VarDecl &VarDecl, const Stmt &Stmt,
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 @@ allDeclRefExprs(const VarDecl &VarDecl, const Stmt &Stmt, ASTContext &Context) {
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();
}

27
clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.h
View File

@ -28,23 +28,34 @@ namespace decl_ref_expr {
bool isOnlyUsedAsConst(const VarDecl &Var, const Stmt &Stmt,
ASTContext &Context);
// Returns set of all DeclRefExprs to VarDecl in Stmt.
/// Returns set of all ``DeclRefExprs`` to ``VarDecl`` within ``Stmt``.
llvm::SmallPtrSet<const DeclRefExpr *, 16>
allDeclRefExprs(const VarDecl &VarDecl, const Stmt &Stmt, ASTContext &Context);
// Returns set of all DeclRefExprs where VarDecl is guaranteed to be accessed in
// a const fashion.
/// Returns set of all ``DeclRefExprs`` to ``VarDecl`` within ``Decl``.
llvm::SmallPtrSet<const DeclRefExpr *, 16>
allDeclRefExprs(const VarDecl &VarDecl, const Decl &Decl, ASTContext &Context);
/// Returns set of all ``DeclRefExprs`` to ``VarDecl`` within ``Stmt`` 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 true if DeclRefExpr is the argument of a copy-constructor call expr.
bool isCopyConstructorArgument(const DeclRefExpr &DeclRef, const Stmt &Stmt,
/// Returns set of all ``DeclRefExprs`` to ``VarDecl`` within ``Decl`` 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 expression within ``Decl``.
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,
/// Returns ``true`` if ``DeclRefExpr`` is the argument of a copy-assignment
/// operator CallExpr within ``Decl``.
bool isCopyAssignmentArgument(const DeclRefExpr &DeclRef, const Decl &Decl,
ASTContext &Context);
} // namespace decl_ref_expr

14
clang-tools-extra/test/clang-tidy/performance-unnecessary-value-param-delayed.cpp
View File

@ -64,6 +64,7 @@ void positiveAndNegative(const ExpensiveToCopyType ConstCopy, const ExpensiveToC
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 @@ void negativeValueNonConstMethodIsCalled(ExpensiveToCopyType Obj) {
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>

26
clang-tools-extra/test/clang-tidy/performance-unnecessary-value-param.cpp
View File

@ -82,6 +82,7 @@ void positiveAndNegative(const ExpensiveToCopyType ConstCopy, const ExpensiveToC
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 @@ void negativeValueNonConstMethodIsCalled(ExpensiveToCopyType Obj) {
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>