[clang-tools-extra] r338903 - Use ExprMutationAnalyzer in performance-unnecessary-value-param
Shuai Wang via cfe-commits
cfe-commits at lists.llvm.org
Fri Aug 3 10:23:37 PDT 2018
Author: shuaiwang
Date: Fri Aug 3 10:23:37 2018
New Revision: 338903
URL: http://llvm.org/viewvc/llvm-project?rev=338903&view=rev
Log:
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
Modified:
clang-tools-extra/trunk/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
clang-tools-extra/trunk/test/clang-tidy/performance-unnecessary-value-param.cpp
Modified: clang-tools-extra/trunk/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/performance/UnnecessaryValueParamCheck.cpp?rev=338903&r1=338902&r2=338903&view=diff
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/performance/UnnecessaryValueParamCheck.cpp (original)
+++ clang-tools-extra/trunk/clang-tidy/performance/UnnecessaryValueParamCheck.cpp Fri Aug 3 10:23:37 2018
@@ -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 N
.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::registe
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 (!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 (!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;
+ }
}
}
+ 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 "
Modified: clang-tools-extra/trunk/test/clang-tidy/performance-unnecessary-value-param.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/performance-unnecessary-value-param.cpp?rev=338903&r1=338902&r2=338903&view=diff
==============================================================================
--- clang-tools-extra/trunk/test/clang-tidy/performance-unnecessary-value-param.cpp (original)
+++ clang-tools-extra/trunk/test/clang-tidy/performance-unnecessary-value-param.cpp Fri Aug 3 10:23:37 2018
@@ -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(ExpensiveToC
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) {
More information about the cfe-commits
mailing list