flx updated this revision to Diff 59157.
flx marked 2 inline comments as done.
http://reviews.llvm.org/D20277
Files:
clang-tidy/performance/UnnecessaryValueParamCheck.cpp
clang-tidy/performance/UnnecessaryValueParamCheck.h
clang-tidy/utils/DeclRefExprUtils.cpp
clang-tidy/utils/DeclRefExprUtils.h
clang-tidy/utils/Matchers.h
clang-tidy/utils/TypeTraits.cpp
clang-tidy/utils/TypeTraits.h
docs/clang-tidy/checks/performance-unnecessary-value-param.rst
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
@@ -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 @@
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 @@
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;
+}
Index: docs/clang-tidy/checks/performance-unnecessary-value-param.rst
===================================================================
--- docs/clang-tidy/checks/performance-unnecessary-value-param.rst
+++ docs/clang-tidy/checks/performance-unnecessary-value-param.rst
@@ -10,7 +10,7 @@
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 @@
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);
+ }
Index: clang-tidy/utils/TypeTraits.h
===================================================================
--- clang-tidy/utils/TypeTraits.h
+++ clang-tidy/utils/TypeTraits.h
@@ -29,6 +29,12 @@
bool recordIsTriviallyDefaultConstructible(const RecordDecl &RecordDecl,
const ASTContext &Context);
+// Returns true if Type has a non-deleted move constructor.
+bool hasMoveConstructor(QualType Type);
+
+// Return true if Type has a non-deleted move assignment operator.
+bool hasMoveAssignmentOperator(QualType Type);
+
} // type_traits
} // namespace utils
} // namespace tidy
Index: clang-tidy/utils/TypeTraits.cpp
===================================================================
--- clang-tidy/utils/TypeTraits.cpp
+++ clang-tidy/utils/TypeTraits.cpp
@@ -124,6 +124,20 @@
return false;
}
+bool hasMoveConstructor(QualType Type) {
+ auto *Record = Type->getAsCXXRecordDecl();
+ if (!Record || !Record->hasDefinition())
+ return false;
+ return Record->hasNonTrivialMoveConstructor();
+}
+
+bool hasMoveAssignmentOperator(QualType Type) {
+ auto *Record = Type->getAsCXXRecordDecl();
+ if (!Record || !Record->hasDefinition())
+ return false;
+ return Record->hasNonTrivialMoveAssignment();
+}
+
} // namespace type_traits
} // namespace utils
} // namespace tidy
Index: clang-tidy/utils/Matchers.h
===================================================================
--- clang-tidy/utils/Matchers.h
+++ clang-tidy/utils/Matchers.h
@@ -45,6 +45,12 @@
Node, Finder->getASTContext());
}
+// Returns QualType matcher for references to const.
+AST_MATCHER_FUNCTION(ast_matchers::TypeMatcher, isReferenceToConst) {
+ return ast_matchers::referenceType(ast_matchers::pointee(
+ ast_matchers::qualType(ast_matchers::isConstQualified())));
+}
+
} // namespace matchers
} // namespace tidy
} // namespace clang
Index: clang-tidy/utils/DeclRefExprUtils.h
===================================================================
--- clang-tidy/utils/DeclRefExprUtils.h
+++ clang-tidy/utils/DeclRefExprUtils.h
@@ -12,6 +12,7 @@
#include "clang/AST/ASTContext.h"
#include "clang/AST/Type.h"
+#include "llvm/ADT/SmallPtrSet.h"
namespace clang {
namespace tidy {
@@ -26,6 +27,25 @@
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
Index: clang-tidy/utils/DeclRefExprUtils.cpp
===================================================================
--- clang-tidy/utils/DeclRefExprUtils.cpp
+++ clang-tidy/utils/DeclRefExprUtils.cpp
@@ -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 @@
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,20 +71,55 @@
return DeclRefs;
}
-} // namespace
-
bool isOnlyUsedAsConst(const VarDecl &Var, const Stmt &Stmt,
ASTContext &Context) {
// Collect all DeclRefExprs to the loop variable and all CallExprs and
// CXXConstructExprs where the loop variable is used as argument to a const
// 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
Index: clang-tidy/performance/UnnecessaryValueParamCheck.h
===================================================================
--- clang-tidy/performance/UnnecessaryValueParamCheck.h
+++ clang-tidy/performance/UnnecessaryValueParamCheck.h
@@ -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 @@
/// 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
Index: clang-tidy/performance/UnnecessaryValueParamCheck.cpp
===================================================================
--- clang-tidy/performance/UnnecessaryValueParamCheck.cpp
+++ clang-tidy/performance/UnnecessaryValueParamCheck.cpp
@@ -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,21 @@
.str();
}
+template <typename S> bool isSetDifferenceEmpty(const S &S1, const S &S2) {
+ for (const auto &E : S1)
+ if (S2.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 +75,39 @@
// 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 (!isSetDifferenceEmpty(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::hasMoveConstructor(CanonicalType) &&
+ utils::decl_ref_expr::isCopyConstructorArgument(
+ **AllDeclRefExprs.begin(), *Function->getBody(),
+ *Result.Context)) ||
+ (utils::type_traits::hasMoveAssignmentOperator(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 +130,40 @@
}
}
+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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
