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

Use ExprMutationAnalyzer in performance-unnecessary-value-param 

Summary:
This yields better recall as ExprMutationAnalyzer is more accurate.

One common pattern this check is now able to catch is:
```
void foo(std::vector<X> v) {
  for (const auto& elm : v) {
    // ...
  }
}
```

Reviewers: george.karpenkov

Subscribers: a.sidorin, cfe-commits

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

llvm-svn: 338903
This commit is contained in:
Shuai Wang 2018-08-03 17:23:37 +00:00
parent cfe5bc158d
commit c2d93d6619
2 changed files with 60 additions and 33 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

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

@ -10,6 +10,7 @@
#include "UnnecessaryValueParamCheck.h"
#include "../utils/DeclRefExprUtils.h"
#include "../utils/ExprMutationAnalyzer.h"
#include "../utils/FixItHintUtils.h"
#include "../utils/Matchers.h"
#include "../utils/TypeTraits.h"
@ -31,14 +32,6 @@ std::string paramNameOrIndex(StringRef Name, size_t Index) {
.str();
}
template <typename S>
bool isSubset(const S &SubsetCandidate, const S &SupersetCandidate) {
for (const auto &E : SubsetCandidate)
if (SupersetCandidate.count(E) == 0)
return false;
return true;
}
bool isReferencedOutsideOfCallExpr(const FunctionDecl &Function,
ASTContext &Context) {
auto Matches = match(declRefExpr(to(functionDecl(equalsNode(&Function))),
@ -98,43 +91,55 @@ void UnnecessaryValueParamCheck::registerMatchers(MatchFinder *Finder) {
void UnnecessaryValueParamCheck::check(const MatchFinder::MatchResult &Result) {
const auto *Param = Result.Nodes.getNodeAs<ParmVarDecl>("param");
const auto *Function = Result.Nodes.getNodeAs<FunctionDecl>("functionDecl");
const size_t Index = std::find(Function->parameters().begin(),
Function->parameters().end(), Param) -
Function->parameters().begin();
bool IsConstQualified =
Param->getType().getCanonicalType().isConstQualified();
auto AllDeclRefExprs = utils::decl_ref_expr::allDeclRefExprs(
*Param, *Function, *Result.Context);
auto ConstDeclRefExprs = utils::decl_ref_expr::constReferenceDeclRefExprs(
*Param, *Function, *Result.Context);
// Do not trigger on non-const value parameters when they are not only used as
// const.
if (!isSubset(AllDeclRefExprs, ConstDeclRefExprs))
// Do not trigger on non-const value parameters when they are mutated either
// within the function body or within init expression(s) when the function is
// a ctor.
if (utils::ExprMutationAnalyzer(Function->getBody(), Result.Context)
.isMutated(Param))
return;
// CXXCtorInitializer might also mutate Param but they're not part of function
// body, so check them separately here.
if (const auto *Ctor = dyn_cast<CXXConstructorDecl>(Function)) {
for (const auto *Init : Ctor->inits()) {
if (utils::ExprMutationAnalyzer(Init->getInit(), Result.Context)
.isMutated(Param))
return;
}
}
const bool IsConstQualified =
Param->getType().getCanonicalType().isConstQualified();
// If the parameter is non-const, check if it has a move constructor and is
// only referenced once to copy-construct another object or whether it has a
// 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 && AllDeclRefExprs.size() == 1) {
auto CanonicalType = Param->getType().getCanonicalType();
const auto &DeclRefExpr = **AllDeclRefExprs.begin();
if (!IsConstQualified) {
auto AllDeclRefExprs = utils::decl_ref_expr::allDeclRefExprs(
*Param, *Function, *Result.Context);
if (AllDeclRefExprs.size() == 1) {
auto CanonicalType = Param->getType().getCanonicalType();
const auto &DeclRefExpr = **AllDeclRefExprs.begin();
if (!hasLoopStmtAncestor(DeclRefExpr, *Function, *Result.Context) &&
((utils::type_traits::hasNonTrivialMoveConstructor(CanonicalType) &&
utils::decl_ref_expr::isCopyConstructorArgument(
DeclRefExpr, *Function, *Result.Context)) ||
(utils::type_traits::hasNonTrivialMoveAssignment(CanonicalType) &&
utils::decl_ref_expr::isCopyAssignmentArgument(
DeclRefExpr, *Function, *Result.Context)))) {
handleMoveFix(*Param, DeclRefExpr, *Result.Context);
return;
if (!hasLoopStmtAncestor(DeclRefExpr, *Function, *Result.Context) &&
((utils::type_traits::hasNonTrivialMoveConstructor(CanonicalType) &&
utils::decl_ref_expr::isCopyConstructorArgument(
DeclRefExpr, *Function, *Result.Context)) ||
(utils::type_traits::hasNonTrivialMoveAssignment(CanonicalType) &&
utils::decl_ref_expr::isCopyAssignmentArgument(
DeclRefExpr, *Function, *Result.Context)))) {
handleMoveFix(*Param, DeclRefExpr, *Result.Context);
return;
}
}
}
const size_t Index = std::find(Function->parameters().begin(),
Function->parameters().end(), Param) -
Function->parameters().begin();
auto Diag =
diag(Param->getLocation(),
IsConstQualified ? "the const qualified parameter %0 is "

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

@ -15,6 +15,20 @@ void mutate(ExpensiveToCopyType *);
void useAsConstReference(const ExpensiveToCopyType &);
void useByValue(ExpensiveToCopyType);
template <class T> class Vector {
public:
using iterator = T*;
using const_iterator = const T*;
Vector(const Vector&);
Vector& operator=(const Vector&);
iterator begin();
iterator end();
const_iterator begin() const;
const_iterator end() const;
};
// This class simulates std::pair<>. It is trivially copy constructible
// and trivially destructible, but not trivially copy assignable.
class SomewhatTrivial {
@ -59,6 +73,14 @@ void positiveExpensiveValue(ExpensiveToCopyType Obj) {
useByValue(Obj);
}
void positiveVector(Vector<ExpensiveToCopyType> V) {
// CHECK-MESSAGES: [[@LINE-1]]:49: warning: the parameter 'V' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param]
// CHECK-FIXES: void positiveVector(const Vector<ExpensiveToCopyType>& V) {
for (const auto& Obj : V) {
useByValue(Obj);
}
}
void positiveWithComment(const ExpensiveToCopyType /* important */ S);
// CHECK-FIXES: void positiveWithComment(const ExpensiveToCopyType& /* important */ S);
void positiveWithComment(const ExpensiveToCopyType /* important */ S) {