flx created this revision.
flx added reviewers: alexfh, sbenza.
flx added a subscriber: cfe-commits.
flx set the repository for this revision to rL LLVM.
Make check more useful in the following two cases:
1. 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.
2. 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.
Repository:
rL LLVM
http://reviews.llvm.org/D20277
Files:
clang-tidy/performance/UnnecessaryValueParamCheck.cpp
clang-tidy/performance/UnnecessaryValueParamCheck.h
clang-tidy/utils/CMakeLists.txt
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
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 &&) = default;
+ ExpensiveMovableType(const ExpensiveMovableType &) = default;
+ ExpensiveMovableType &operator=(const ExpensiveMovableType &) = default;
+ ExpensiveMovableType &operator=(ExpensiveMovableType &&) = default;
+ ~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: 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
@@ -123,6 +123,28 @@
return false;
}
+bool hasMoveConstructor(QualType Type) {
+ auto *Record = Type->getAsCXXRecordDecl();
+ if (!Record || !Record->hasDefinition())
+ return false;
+ for (const auto *Constructor : Record->ctors()) {
+ if (Constructor->isMoveConstructor() && !Constructor->isDeleted())
+ return true;
+ }
+ return false;
+}
+
+bool hasMoveAssignmentOperator(QualType Type) {
+ auto *Record = Type->getAsCXXRecordDecl();
+ if (!Record || !Record->hasDefinition())
+ return false;
+ for (const auto *Method : Record->methods()) {
+ if (Method->isMoveAssignmentOperator() && !Method->isDeleted())
+ return true;
+ }
+ return false;
+}
+
} // namespace type_traits
} // namespace utils
} // namespace tidy
Index: clang-tidy/utils/Matchers.h
===================================================================
--- clang-tidy/utils/Matchers.h
+++ clang-tidy/utils/Matchers.h
@@ -40,6 +40,9 @@
Node, Finder->getASTContext());
}
+// Returns QualType matcher for references to const.
+ast_matchers::TypeMatcher isConstReference();
+
} // 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,19 @@
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 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,17 +38,6 @@
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;
-}
-
// 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>
@@ -89,11 +78,48 @@
// 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::isConstReference())));
+ auto Matches =
+ match(findAll(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::isConstReference())));
+ auto Matches =
+ match(findAll(cxxOperatorCallExpr(UsedAsConstRefArg,
+ hasOverloadedOperatorName("="))
+ .bind("operatorCallExpr")),
+ Stmt, Context);
+ return !Matches.empty();
+}
+
} // namespace decl_ref_expr
} // namespace utils
} // namespace tidy
Index: clang-tidy/utils/CMakeLists.txt
===================================================================
--- clang-tidy/utils/CMakeLists.txt
+++ clang-tidy/utils/CMakeLists.txt
@@ -8,6 +8,7 @@
IncludeInserter.cpp
IncludeSorter.cpp
LexerUtils.cpp
+ Matchers.cpp
OptionsUtils.cpp
TypeTraits.cpp
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,19 @@
/// 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(clang::CompilerInstance &Compiler) override;
+ void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
+
+private:
+ void handleMoveFix(const ParmVarDecl &Var,
+ const DeclRefExpr &CopyArgument,
+ const SourceManager &SM);
+
+ 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,9 @@
#include "../utils/DeclRefExprUtils.h"
#include "../utils/FixItHintUtils.h"
#include "../utils/Matchers.h"
+#include "../utils/TypeTraits.h"
+#include "clang/Frontend/CompilerInstance.h"
+#include "clang/Lex/Preprocessor.h"
using namespace clang::ast_matchers;
@@ -29,6 +32,12 @@
} // 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(),
@@ -64,6 +73,28 @@
!utils::decl_ref_expr::isOnlyUsedAsConst(
*Param, *Function->getBody(), *Result.Context)))
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 Matches = utils::decl_ref_expr::allDeclRefExprs(
+ *Param, *Function->getBody(), *Result.Context);
+ auto CanonicalType = Param->getType().getCanonicalType();
+ if (Matches.size() == 1 &&
+ ((utils::type_traits::hasMoveConstructor(CanonicalType) &&
+ utils::decl_ref_expr::isCopyConstructorArgument(
+ **Matches.begin(), *Function->getBody(), *Result.Context)) ||
+ (utils::type_traits::hasMoveAssignmentOperator(CanonicalType) &&
+ utils::decl_ref_expr::isCopyAssignmentArgument(
+ **Matches.begin(), *Function->getBody(), *Result.Context)))) {
+ handleMoveFix(*Param, **Matches.begin(), *Result.SourceManager);
+ return;
+ }
+ }
+
auto Diag =
diag(Param->getLocation(),
IsConstQualified ? "the const qualified parameter %0 is "
@@ -86,6 +117,38 @@
}
}
+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 SourceManager &SM) {
+ 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;
+ Diag << FixItHint::CreateReplacement(
+ CopyArgument.getSourceRange(),
+ (Twine("std::move(") + Var.getName() + ")").str());
+ 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
