flx created this revision.
flx added reviewers: alexfh, sbenza, aaron.ballman.
flx added a subscriber: cfe-commits.
Add matcher that identifies arguments that are passed by value, are copy
assigned to a class field and have a non-deleted move constructor. If the type
is also expensive to copy issue diagnostic message that the field can be move
constructed.
http://reviews.llvm.org/D12839
Files:
clang-tidy/misc/MoveConstructorInitCheck.cpp
clang-tidy/misc/MoveConstructorInitCheck.h
test/clang-tidy/misc-move-constructor-init.cpp
Index: test/clang-tidy/misc-move-constructor-init.cpp
===================================================================
--- test/clang-tidy/misc-move-constructor-init.cpp
+++ test/clang-tidy/misc-move-constructor-init.cpp
@@ -76,3 +76,60 @@
B Mem;
N(N &&RHS) : Mem(move(RHS.Mem)) {}
};
+
+
+struct Movable {
+ Movable(Movable&&) = default;
+ Movable(const Movable&) = default;
+ Movable& operator =(const Movable&) = default;
+ ~Movable() {}
+};
+
+struct TriviallyCopyable {
+ TriviallyCopyable() = default;
+ TriviallyCopyable(TriviallyCopyable&&) = default;
+ TriviallyCopyable(const TriviallyCopyable&) = default;
+};
+
+struct Positive {
+ Positive(Movable M) : M_(M) {}
+ // CHECK-MESSAGES: [[@LINE-1]]:28: warning: value parameter can be moved to avoid copy. [misc-move-constructor-init]
+ Movable M_;
+};
+
+struct NegativeMultipleInitializerReferences {
+ NegativeMultipleInitializerReferences(Movable M) : M_(M), n_(M) {}
+ Movable M_;
+ Movable n_;
+};
+
+struct NegativeReferencedInConstructorBody {
+ NegativeReferencedInConstructorBody(Movable M) : M_(M) {
+ M_ = M;
+ }
+ Movable M_;
+};
+
+struct NegativeParamTriviallyCopyable {
+ NegativeParamTriviallyCopyable(TriviallyCopyable T) : T_(T) {}
+ TriviallyCopyable T_;
+};
+
+struct NegativeNotPassedByValue {
+ NegativeNotPassedByValue(const Movable& M) : M_(M) {}
+ NegativeNotPassedByValue(const Movable M) : M_(M) {}
+ NegativeNotPassedByValue(Movable& M) : M_(M) {}
+ NegativeNotPassedByValue(Movable* M) : M_(*M) {}
+ NegativeNotPassedByValue(const Movable* M) : M_(*M) {}
+ Movable M_;
+};
+
+struct Immovable {
+ Immovable(const Immovable&) = default;
+ Immovable(Immovable&&) = delete;
+};
+
+struct NegativeImmovableParameter {
+ NegativeImmovableParameter(Immovable I) : I_(I) {}
+ Immovable I_;
+};
Index: clang-tidy/misc/MoveConstructorInitCheck.h
===================================================================
--- clang-tidy/misc/MoveConstructorInitCheck.h
+++ clang-tidy/misc/MoveConstructorInitCheck.h
@@ -18,12 +18,21 @@
/// The check flags user-defined move constructors that have a ctor-initializer
/// initializing a member or base class through a copy constructor instead of a
/// move constructor.
+/// It also flags constructor arguments that are passed by value, have a
+/// non-deleted move-constructor and are assigned to a class field by copy
+/// construction.
class MoveConstructorInitCheck : public ClangTidyCheck {
public:
MoveConstructorInitCheck(StringRef Name, ClangTidyContext *Context)
: ClangTidyCheck(Name, Context) {}
void registerMatchers(ast_matchers::MatchFinder *Finder) override;
void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+private:
+ void
+ handleMoveConstructor(const ast_matchers::MatchFinder::MatchResult &Result);
+ void
+ handleParamNotMoved(const ast_matchers::MatchFinder::MatchResult &Result);
};
} // namespace tidy
Index: clang-tidy/misc/MoveConstructorInitCheck.cpp
===================================================================
--- clang-tidy/misc/MoveConstructorInitCheck.cpp
+++ clang-tidy/misc/MoveConstructorInitCheck.cpp
@@ -16,6 +16,48 @@
namespace clang {
namespace tidy {
+namespace {
+
+bool classHasTrivialCopyAndDestroy(QualType Type) {
+ auto *Record = Type->getAsCXXRecordDecl();
+ return Record && Record->hasDefinition() &&
+ !Record->hasNonTrivialCopyConstructor() &&
+ !Record->hasNonTrivialDestructor();
+}
+
+AST_MATCHER(QualType, isExpensiveToCopy) {
+ // We can't reason about dependent types. Ignore them.
+ // If there's a template instantiation that results in what we consider
+ // excessive copying, we'll warn on them.
+ if (Node->isDependentType()) {
+ return false;
+ }
+ // Ignore trivially copyable types.
+ if (Node->isScalarType() ||
+ Node.isTriviallyCopyableType(Finder->getASTContext()) ||
+ classHasTrivialCopyAndDestroy(Node)) {
+ return false;
+ }
+ return true;
+}
+
+int parmVarDeclRefExprOccurences(const ParmVarDecl &MovableParam,
+ const CXXConstructorDecl &ConstructorDecl,
+ ASTContext &Context) {
+ int Occurrences = 0;
+ auto AllDeclRefs =
+ findAll(declRefExpr(to(parmVarDecl(equalsNode(&MovableParam)))));
+ auto Matches = match(AllDeclRefs, *ConstructorDecl.getBody(), Context);
+ Occurrences += Matches.size();
+ for (const auto* Initializer : ConstructorDecl.inits()) {
+ Matches = match(AllDeclRefs, *Initializer->getInit(), Context);
+ Occurrences += Matches.size();
+ }
+ return Occurrences;
+}
+
+} // namespace
+
void MoveConstructorInitCheck::registerMatchers(MatchFinder *Finder) {
// Only register the matchers for C++11; the functionality currently does not
// provide any benefit to other languages, despite being benign.
@@ -28,14 +70,60 @@
hasAnyConstructorInitializer(
ctorInitializer(withInitializer(constructExpr(hasDeclaration(
constructorDecl(isCopyConstructor()).bind("ctor")
- )))).bind("init")
+ )))).bind("move-init")
)
)), this);
+
+ auto NonConstValueMovableAndExpensiveToCopy =
+ qualType(allOf(unless(pointerType()), unless(isConstQualified()),
+ hasDeclaration(recordDecl(hasMethod(constructorDecl(
+ isMoveConstructor(), unless(isDeleted()))))),
+ isExpensiveToCopy()));
+ Finder->addMatcher(
+ constructorDecl(
+ allOf(
+ unless(isMoveConstructor()),
+ hasAnyConstructorInitializer(withInitializer(constructExpr(
+ hasDeclaration(constructorDecl(isCopyConstructor())),
+ hasArgument(
+ 0, declRefExpr(
+ to(parmVarDecl(
+ hasType(
+ NonConstValueMovableAndExpensiveToCopy))
+ .bind("movable-param")))
+ .bind("init-arg")))))))
+ .bind("ctor-decl"),
+ this);
}
void MoveConstructorInitCheck::check(const MatchFinder::MatchResult &Result) {
+ if (Result.Nodes.getNodeAs<CXXCtorInitializer>("move-init") != nullptr) {
+ handleMoveConstructor(Result);
+ }
+ if (Result.Nodes.getNodeAs<ParmVarDecl>("movable-param") != nullptr) {
+ handleParamNotMoved(Result);
+ }
+}
+
+void MoveConstructorInitCheck::handleParamNotMoved(
+ const MatchFinder::MatchResult &Result) {
+ const auto *MovableParam =
+ Result.Nodes.getNodeAs<ParmVarDecl>("movable-param");
+ const auto *ConstructorDecl =
+ Result.Nodes.getNodeAs<CXXConstructorDecl>("ctor-decl");
+ const auto *InitArg = Result.Nodes.getNodeAs<DeclRefExpr>("init-arg");
+ // If the parameter is referenced more than once it is not safe to move it.
+ if (parmVarDeclRefExprOccurences(*MovableParam, *ConstructorDecl,
+ *Result.Context) > 1) {
+ return;
+ }
+ diag(InitArg->getLocStart(), "value parameter can be moved to avoid copy.");
+}
+
+void MoveConstructorInitCheck::handleMoveConstructor(
+ const MatchFinder::MatchResult &Result) {
const auto *CopyCtor = Result.Nodes.getNodeAs<CXXConstructorDecl>("ctor");
- const auto *Initializer = Result.Nodes.getNodeAs<CXXCtorInitializer>("init");
+ const auto *Initializer = Result.Nodes.getNodeAs<CXXCtorInitializer>("move-init");
// Do not diagnose if the expression used to perform the initialization is a
// trivially-copyable type.
@@ -79,4 +167,3 @@
} // namespace tidy
} // namespace clang
-
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
