[clang-tools-extra] r274380 - [clang-tidy] UnnecessaryValueParamCheck - suggest std::move() if non-const value parameter can be moved.
Felix Berger via cfe-commits
cfe-commits at lists.llvm.org
Fri Jul 1 13:12:15 PDT 2016
Author: flx
Date: Fri Jul 1 15:12:15 2016
New Revision: 274380
URL: http://llvm.org/viewvc/llvm-project?rev=274380&view=rev
Log:
[clang-tidy] UnnecessaryValueParamCheck - suggest std::move() if non-const value parameter can be moved.
Summary:
Make check more useful in the following two cases:
The parameter is passed by non-const value, has a non-deleted move constructor and is only referenced once in the function as argument to the type's copy constructor.
The parameter is passed by non-const value, has a non-deleted move assignment operator and is only referenced once in the function as argument of the the type's copy assignment operator.
In this case suggest a fix to move the parameter which avoids the unnecessary copy and is closest to what the user might have intended.
Reviewers: alexfh, sbenza
Subscribers: cfe-commits, Prazek
Differential Revision: http://reviews.llvm.org/D20277
Modified:
clang-tools-extra/trunk/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
clang-tools-extra/trunk/clang-tidy/performance/UnnecessaryValueParamCheck.h
clang-tools-extra/trunk/clang-tidy/utils/DeclRefExprUtils.cpp
clang-tools-extra/trunk/clang-tidy/utils/DeclRefExprUtils.h
clang-tools-extra/trunk/clang-tidy/utils/Matchers.h
clang-tools-extra/trunk/clang-tidy/utils/TypeTraits.cpp
clang-tools-extra/trunk/clang-tidy/utils/TypeTraits.h
clang-tools-extra/trunk/docs/clang-tidy/checks/performance-unnecessary-value-param.rst
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=274380&r1=274379&r2=274380&view=diff
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/performance/UnnecessaryValueParamCheck.cpp (original)
+++ clang-tools-extra/trunk/clang-tidy/performance/UnnecessaryValueParamCheck.cpp Fri Jul 1 15:12:15 2016
@@ -12,6 +12,10 @@
#include "../utils/DeclRefExprUtils.h"
#include "../utils/FixItHintUtils.h"
#include "../utils/Matchers.h"
+#include "../utils/TypeTraits.h"
+#include "clang/Frontend/CompilerInstance.h"
+#include "clang/Lex/Lexer.h"
+#include "clang/Lex/Preprocessor.h"
using namespace clang::ast_matchers;
@@ -27,8 +31,22 @@ 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;
+}
+
} // namespace
+UnnecessaryValueParamCheck::UnnecessaryValueParamCheck(
+ StringRef Name, ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context),
+ IncludeStyle(utils::IncludeSorter::parseIncludeStyle(
+ Options.get("IncludeStyle", "llvm"))) {}
+
void UnnecessaryValueParamCheck::registerMatchers(MatchFinder *Finder) {
const auto ExpensiveValueParamDecl =
parmVarDecl(hasType(hasCanonicalType(allOf(matchers::isExpensiveToCopy(),
@@ -58,12 +76,39 @@ void UnnecessaryValueParamCheck::check(c
// Do not trigger on non-const value parameters when:
// 1. they are in a constructor definition since they can likely trigger
// misc-move-constructor-init which will suggest to move the argument.
- // 2. they are not only used as const.
if (!IsConstQualified && (llvm::isa<CXXConstructorDecl>(Function) ||
- !Function->doesThisDeclarationHaveABody() ||
- !utils::decl_ref_expr::isOnlyUsedAsConst(
- *Param, *Function->getBody(), *Result.Context)))
+ !Function->doesThisDeclarationHaveABody()))
return;
+
+ auto AllDeclRefExprs = utils::decl_ref_expr::allDeclRefExprs(
+ *Param, *Function->getBody(), *Result.Context);
+ auto ConstDeclRefExprs = utils::decl_ref_expr::constReferenceDeclRefExprs(
+ *Param, *Function->getBody(), *Result.Context);
+ // 2. they are not only used as const.
+ if (!isSubset(AllDeclRefExprs, ConstDeclRefExprs))
+ return;
+
+ // 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) {
+ auto CanonicalType = Param->getType().getCanonicalType();
+ if (AllDeclRefExprs.size() == 1 &&
+ ((utils::type_traits::hasNonTrivialMoveConstructor(CanonicalType) &&
+ utils::decl_ref_expr::isCopyConstructorArgument(
+ **AllDeclRefExprs.begin(), *Function->getBody(),
+ *Result.Context)) ||
+ (utils::type_traits::hasNonTrivialMoveAssignment(CanonicalType) &&
+ utils::decl_ref_expr::isCopyAssignmentArgument(
+ **AllDeclRefExprs.begin(), *Function->getBody(),
+ *Result.Context)))) {
+ handleMoveFix(*Param, **AllDeclRefExprs.begin(), *Result.Context);
+ return;
+ }
+ }
+
auto Diag =
diag(Param->getLocation(),
IsConstQualified ? "the const qualified parameter %0 is "
@@ -86,6 +131,40 @@ void UnnecessaryValueParamCheck::check(c
}
}
+void UnnecessaryValueParamCheck::registerPPCallbacks(
+ CompilerInstance &Compiler) {
+ Inserter.reset(new utils::IncludeInserter(
+ Compiler.getSourceManager(), Compiler.getLangOpts(), IncludeStyle));
+ Compiler.getPreprocessor().addPPCallbacks(Inserter->CreatePPCallbacks());
+}
+
+void UnnecessaryValueParamCheck::storeOptions(
+ ClangTidyOptions::OptionMap &Opts) {
+ Options.store(Opts, "IncludeStyle",
+ utils::IncludeSorter::toString(IncludeStyle));
+}
+
+void UnnecessaryValueParamCheck::handleMoveFix(const ParmVarDecl &Var,
+ const DeclRefExpr &CopyArgument,
+ const ASTContext &Context) {
+ auto Diag = diag(CopyArgument.getLocStart(),
+ "parameter %0 is passed by value and only copied once; "
+ "consider moving it to avoid unnecessary copies")
+ << &Var;
+ // Do not propose fixes in macros since we cannot place them correctly.
+ if (CopyArgument.getLocStart().isMacroID())
+ return;
+ const auto &SM = Context.getSourceManager();
+ auto EndLoc = Lexer::getLocForEndOfToken(CopyArgument.getLocation(), 0, SM,
+ Context.getLangOpts());
+ Diag << FixItHint::CreateInsertion(CopyArgument.getLocStart(), "std::move(")
+ << FixItHint::CreateInsertion(EndLoc, ")");
+ if (auto IncludeFixit = Inserter->CreateIncludeInsertion(
+ SM.getFileID(CopyArgument.getLocStart()), "utility",
+ /*IsAngled=*/true))
+ Diag << *IncludeFixit;
+}
+
} // namespace performance
} // namespace tidy
} // namespace clang
Modified: clang-tools-extra/trunk/clang-tidy/performance/UnnecessaryValueParamCheck.h
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/performance/UnnecessaryValueParamCheck.h?rev=274380&r1=274379&r2=274380&view=diff
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/performance/UnnecessaryValueParamCheck.h (original)
+++ clang-tools-extra/trunk/clang-tidy/performance/UnnecessaryValueParamCheck.h Fri Jul 1 15:12:15 2016
@@ -11,6 +11,7 @@
#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_UNNECESSARY_VALUE_PARAM_H
#include "../ClangTidy.h"
+#include "../utils/IncludeInserter.h"
namespace clang {
namespace tidy {
@@ -23,10 +24,18 @@ namespace performance {
/// http://clang.llvm.org/extra/clang-tidy/checks/performance-unnecessary-value-param.html
class UnnecessaryValueParamCheck : public ClangTidyCheck {
public:
- UnnecessaryValueParamCheck(StringRef Name, ClangTidyContext *Context)
- : ClangTidyCheck(Name, Context) {}
+ UnnecessaryValueParamCheck(StringRef Name, ClangTidyContext *Context);
void registerMatchers(ast_matchers::MatchFinder *Finder) override;
void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+ void registerPPCallbacks(CompilerInstance &Compiler) override;
+ void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
+
+private:
+ void handleMoveFix(const ParmVarDecl &Var, const DeclRefExpr &CopyArgument,
+ const ASTContext &Context);
+
+ std::unique_ptr<utils::IncludeInserter> Inserter;
+ const utils::IncludeSorter::IncludeStyle IncludeStyle;
};
} // namespace performance
Modified: clang-tools-extra/trunk/clang-tidy/utils/DeclRefExprUtils.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/utils/DeclRefExprUtils.cpp?rev=274380&r1=274379&r2=274380&view=diff
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/utils/DeclRefExprUtils.cpp (original)
+++ clang-tools-extra/trunk/clang-tidy/utils/DeclRefExprUtils.cpp Fri Jul 1 15:12:15 2016
@@ -8,10 +8,10 @@
//===----------------------------------------------------------------------===//
#include "DeclRefExprUtils.h"
+#include "Matchers.h"
#include "clang/AST/ASTContext.h"
#include "clang/AST/DeclCXX.h"
#include "clang/ASTMatchers/ASTMatchFinder.h"
-#include "llvm/ADT/SmallPtrSet.h"
namespace clang {
namespace tidy {
@@ -38,16 +38,7 @@ void extractNodesByIdTo(ArrayRef<BoundNo
Nodes.insert(Match.getNodeAs<Node>(ID));
}
-// Finds all DeclRefExprs to VarDecl in Stmt.
-SmallPtrSet<const DeclRefExpr *, 16>
-declRefExprs(const VarDecl &VarDecl, const Stmt &Stmt, ASTContext &Context) {
- auto Matches = match(
- findAll(declRefExpr(to(varDecl(equalsNode(&VarDecl)))).bind("declRef")),
- Stmt, Context);
- SmallPtrSet<const DeclRefExpr *, 16> DeclRefs;
- extractNodesByIdTo(Matches, "declRef", DeclRefs);
- return DeclRefs;
-}
+} // namespace
// 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.
@@ -80,8 +71,6 @@ constReferenceDeclRefExprs(const VarDecl
return DeclRefs;
}
-} // namespace
-
bool isOnlyUsedAsConst(const VarDecl &Var, const Stmt &Stmt,
ASTContext &Context) {
// Collect all DeclRefExprs to the loop variable and all CallExprs and
@@ -89,11 +78,48 @@ bool isOnlyUsedAsConst(const VarDecl &Va
// reference parameter.
// If the difference is empty it is safe for the loop variable to be a const
// reference.
- auto AllDeclRefs = declRefExprs(Var, Stmt, Context);
+ auto AllDeclRefs = allDeclRefExprs(Var, Stmt, Context);
auto ConstReferenceDeclRefs = constReferenceDeclRefExprs(Var, Stmt, Context);
return isSetDifferenceEmpty(AllDeclRefs, ConstReferenceDeclRefs);
}
+SmallPtrSet<const DeclRefExpr *, 16>
+allDeclRefExprs(const VarDecl &VarDecl, const Stmt &Stmt, ASTContext &Context) {
+ auto Matches = match(
+ findAll(declRefExpr(to(varDecl(equalsNode(&VarDecl)))).bind("declRef")),
+ Stmt, Context);
+ SmallPtrSet<const DeclRefExpr *, 16> DeclRefs;
+ extractNodesByIdTo(Matches, "declRef", DeclRefs);
+ return DeclRefs;
+}
+
+bool isCopyConstructorArgument(const DeclRefExpr &DeclRef, const Stmt &Stmt,
+ ASTContext &Context) {
+ auto UsedAsConstRefArg = forEachArgumentWithParam(
+ declRefExpr(equalsNode(&DeclRef)),
+ parmVarDecl(hasType(matchers::isReferenceToConst())));
+ auto Matches = match(
+ stmt(hasDescendant(
+ cxxConstructExpr(UsedAsConstRefArg, hasDeclaration(cxxConstructorDecl(
+ isCopyConstructor())))
+ .bind("constructExpr"))),
+ Stmt, Context);
+ return !Matches.empty();
+}
+
+bool isCopyAssignmentArgument(const DeclRefExpr &DeclRef, const Stmt &Stmt,
+ ASTContext &Context) {
+ auto UsedAsConstRefArg = forEachArgumentWithParam(
+ declRefExpr(equalsNode(&DeclRef)),
+ parmVarDecl(hasType(matchers::isReferenceToConst())));
+ auto Matches = match(
+ stmt(hasDescendant(
+ cxxOperatorCallExpr(UsedAsConstRefArg, hasOverloadedOperatorName("="))
+ .bind("operatorCallExpr"))),
+ Stmt, Context);
+ return !Matches.empty();
+}
+
} // namespace decl_ref_expr
} // namespace utils
} // namespace tidy
Modified: clang-tools-extra/trunk/clang-tidy/utils/DeclRefExprUtils.h
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/utils/DeclRefExprUtils.h?rev=274380&r1=274379&r2=274380&view=diff
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/utils/DeclRefExprUtils.h (original)
+++ clang-tools-extra/trunk/clang-tidy/utils/DeclRefExprUtils.h Fri Jul 1 15:12:15 2016
@@ -12,6 +12,7 @@
#include "clang/AST/ASTContext.h"
#include "clang/AST/Type.h"
+#include "llvm/ADT/SmallPtrSet.h"
namespace clang {
namespace tidy {
@@ -27,6 +28,25 @@ namespace decl_ref_expr {
bool isOnlyUsedAsConst(const VarDecl &Var, const Stmt &Stmt,
ASTContext &Context);
+// Returns set of all DeclRefExprs to VarDecl in 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.
+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,
+ ASTContext &Context);
+
+// Returns true if DeclRefExpr is the argument of a copy-assignment operator
+// call expr.
+bool isCopyAssignmentArgument(const DeclRefExpr &DeclRef, const Stmt &Stmt,
+ ASTContext &Context);
+
} // namespace decl_ref_expr
} // namespace utils
} // namespace tidy
Modified: clang-tools-extra/trunk/clang-tidy/utils/Matchers.h
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/utils/Matchers.h?rev=274380&r1=274379&r2=274380&view=diff
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/utils/Matchers.h (original)
+++ clang-tools-extra/trunk/clang-tidy/utils/Matchers.h Fri Jul 1 15:12:15 2016
@@ -42,6 +42,12 @@ AST_MATCHER(RecordDecl, isTriviallyDefau
AST_MATCHER(FieldDecl, isBitfield) { return Node.isBitField(); }
+// Returns QualType matcher for references to const.
+AST_MATCHER_FUNCTION(ast_matchers::TypeMatcher, isReferenceToConst) {
+ using namespace ast_matchers;
+ return referenceType(pointee(qualType(isConstQualified())));
+}
+
} // namespace matchers
} // namespace tidy
} // namespace clang
Modified: clang-tools-extra/trunk/clang-tidy/utils/TypeTraits.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/utils/TypeTraits.cpp?rev=274380&r1=274379&r2=274380&view=diff
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/utils/TypeTraits.cpp (original)
+++ clang-tools-extra/trunk/clang-tidy/utils/TypeTraits.cpp Fri Jul 1 15:12:15 2016
@@ -124,6 +124,18 @@ bool isTriviallyDefaultConstructible(Qua
return false;
}
+bool hasNonTrivialMoveConstructor(QualType Type) {
+ auto *Record = Type->getAsCXXRecordDecl();
+ return Record && Record->hasDefinition() &&
+ Record->hasNonTrivialMoveConstructor();
+}
+
+bool hasNonTrivialMoveAssignment(QualType Type) {
+ auto *Record = Type->getAsCXXRecordDecl();
+ return Record && Record->hasDefinition() &&
+ Record->hasNonTrivialMoveAssignment();
+}
+
} // namespace type_traits
} // namespace utils
} // namespace tidy
Modified: clang-tools-extra/trunk/clang-tidy/utils/TypeTraits.h
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/utils/TypeTraits.h?rev=274380&r1=274379&r2=274380&view=diff
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/utils/TypeTraits.h (original)
+++ clang-tools-extra/trunk/clang-tidy/utils/TypeTraits.h Fri Jul 1 15:12:15 2016
@@ -29,6 +29,12 @@ bool isTriviallyDefaultConstructible(Qua
bool recordIsTriviallyDefaultConstructible(const RecordDecl &RecordDecl,
const ASTContext &Context);
+/// Returns true if `Type` has a non-trivial move constructor.
+bool hasNonTrivialMoveConstructor(QualType Type);
+
+/// Return true if `Type` has a non-trivial move assignment operator.
+bool hasNonTrivialMoveAssignment(QualType Type);
+
} // type_traits
} // namespace utils
} // namespace tidy
Modified: clang-tools-extra/trunk/docs/clang-tidy/checks/performance-unnecessary-value-param.rst
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/docs/clang-tidy/checks/performance-unnecessary-value-param.rst?rev=274380&r1=274379&r2=274380&view=diff
==============================================================================
--- clang-tools-extra/trunk/docs/clang-tidy/checks/performance-unnecessary-value-param.rst (original)
+++ clang-tools-extra/trunk/docs/clang-tidy/checks/performance-unnecessary-value-param.rst Fri Jul 1 15:12:15 2016
@@ -10,7 +10,7 @@ The check is only applied to parameters
which means they are not trivially copyable or have a non-trivial copy
constructor or destructor.
-To ensure that it is safe to replace the value paramater with a const reference
+To ensure that it is safe to replace the value parameter with a const reference
the following heuristic is employed:
1. the parameter is const qualified;
@@ -31,3 +31,25 @@ Example:
Value.ConstMethd();
ExpensiveToCopy Copy(Value);
}
+
+If the parameter is not const, only copied or assigned once and has a
+non-trivial move-constructor or move-assignment operator respectively the check
+will suggest to move it.
+
+Example:
+
+.. code-block:: c++
+
+ void setValue(string Value) {
+ Field = Value;
+ }
+
+Will become:
+
+.. code-block:: c++
+
+ #include <utility>
+
+ void setValue(string Value) {
+ Field = std::move(Value);
+ }
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=274380&r1=274379&r2=274380&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 Jul 1 15:12:15 2016
@@ -1,5 +1,7 @@
// RUN: %check_clang_tidy %s performance-unnecessary-value-param %t
+// CHECK-FIXES: #include <utility>
+
struct ExpensiveToCopyType {
const ExpensiveToCopyType & constReference() const {
return *this;
@@ -30,6 +32,15 @@ struct MoveOnlyType {
void constMethod() const;
};
+struct ExpensiveMovableType {
+ ExpensiveMovableType();
+ ExpensiveMovableType(ExpensiveMovableType &&);
+ ExpensiveMovableType(const ExpensiveMovableType &) = default;
+ ExpensiveMovableType &operator=(const ExpensiveMovableType &) = default;
+ ExpensiveMovableType &operator=(ExpensiveMovableType &&);
+ ~ExpensiveMovableType();
+};
+
void positiveExpensiveConstValue(const ExpensiveToCopyType Obj);
// CHECK-FIXES: void positiveExpensiveConstValue(const ExpensiveToCopyType& Obj);
void positiveExpensiveConstValue(const ExpensiveToCopyType Obj) {
@@ -180,3 +191,36 @@ struct NegativeDeletedMethod {
void NegativeMoveOnlyTypePassedByValue(MoveOnlyType M) {
M.constMethod();
}
+
+void PositiveMoveOnCopyConstruction(ExpensiveMovableType E) {
+ auto F = E;
+ // CHECK-MESSAGES: [[@LINE-1]]:12: warning: parameter 'E' is passed by value and only copied once; consider moving it to avoid unnecessary copies [performance-unnecessary-value-param]
+ // CHECK-FIXES: auto F = std::move(E);
+}
+
+void PositiveConstRefNotMoveSinceReferencedMultipleTimes(ExpensiveMovableType E) {
+ // CHECK-MESSAGES: [[@LINE-1]]:79: warning: the parameter 'E' is copied
+ // CHECK-FIXES: void PositiveConstRefNotMoveSinceReferencedMultipleTimes(const ExpensiveMovableType& E) {
+ auto F = E;
+ auto G = E;
+}
+
+void PositiveMoveOnCopyAssignment(ExpensiveMovableType E) {
+ ExpensiveMovableType F;
+ F = E;
+ // CHECK-MESSAGES: [[@LINE-1]]:7: warning: parameter 'E' is passed by value
+ // CHECK-FIXES: F = std::move(E);
+}
+
+void PositiveConstRefNotMoveConstructible(ExpensiveToCopyType T) {
+ // CHECK-MESSAGES: [[@LINE-1]]:63: warning: the parameter 'T' is copied
+ // CHECK-FIXES: void PositiveConstRefNotMoveConstructible(const ExpensiveToCopyType& T) {
+ auto U = T;
+}
+
+void PositiveConstRefNotMoveAssignable(ExpensiveToCopyType A) {
+ // CHECK-MESSAGES: [[@LINE-1]]:60: warning: the parameter 'A' is copied
+ // CHECK-FIXES: void PositiveConstRefNotMoveAssignable(const ExpensiveToCopyType& A) {
+ ExpensiveToCopyType B;
+ B = A;
+}
More information about the cfe-commits
mailing list