Index: clang-tidy/misc/MoveConstructorInitCheck.h =================================================================== --- clang-tidy/misc/MoveConstructorInitCheck.h +++ clang-tidy/misc/MoveConstructorInitCheck.h @@ -18,12 +18,20 @@ /// 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. +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/misc-move-constructor-init.html 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 @@ -8,6 +8,7 @@ //===----------------------------------------------------------------------===// #include "MoveConstructorInitCheck.h" +#include "../utils/Matchers.h" #include "clang/AST/ASTContext.h" #include "clang/ASTMatchers/ASTMatchFinder.h" @@ -16,6 +17,23 @@ namespace clang { namespace tidy { +namespace { + +unsigned int parmVarDeclRefExprOccurences(const ParmVarDecl &MovableParam, + const CXXConstructorDecl &ConstructorDecl, + ASTContext &Context) { + unsigned int Occurrences = 0; + auto AllDeclRefs = + findAll(declRefExpr(to(parmVarDecl(equalsNode(&MovableParam))))); + Occurrences += match(AllDeclRefs, *ConstructorDecl.getBody(), Context).size(); + for (const auto *Initializer : ConstructorDecl.inits()) { + Occurrences += match(AllDeclRefs, *Initializer->getInit(), Context).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 +46,57 @@ 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()))))), + matchers::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("move-init") != nullptr) + handleMoveConstructor(Result); + if (Result.Nodes.getNodeAs("movable-param") != nullptr) + handleParamNotMoved(Result); +} + +void MoveConstructorInitCheck::handleParamNotMoved( + const MatchFinder::MatchResult &Result) { + const auto *MovableParam = + Result.Nodes.getNodeAs("movable-param"); + const auto *ConstructorDecl = + Result.Nodes.getNodeAs("ctor-decl"); + const auto *InitArg = Result.Nodes.getNodeAs("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 argument can be moved to avoid copy"); +} + +void MoveConstructorInitCheck::handleMoveConstructor( + const MatchFinder::MatchResult &Result) { const auto *CopyCtor = Result.Nodes.getNodeAs("ctor"); - const auto *Initializer = Result.Nodes.getNodeAs("init"); + const auto *Initializer = Result.Nodes.getNodeAs("move-init"); // Do not diagnose if the expression used to perform the initialization is a // trivially-copyable type. @@ -79,4 +140,3 @@ } // namespace tidy } // namespace clang - Index: clang-tidy/utils/CMakeLists.txt =================================================================== --- clang-tidy/utils/CMakeLists.txt +++ clang-tidy/utils/CMakeLists.txt @@ -4,6 +4,7 @@ HeaderGuard.cpp IncludeInserter.cpp IncludeSorter.cpp + TypeTraits.cpp LINK_LIBS clangAST Index: clang-tidy/utils/Matchers.h =================================================================== --- clang-tidy/utils/Matchers.h +++ clang-tidy/utils/Matchers.h @@ -0,0 +1,23 @@ +//===--- Matchers.h - clang-tidy-------------------------------------------===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// + +#include "clang/ASTMatchers/ASTMatchers.h" +#include "TypeTraits.h" + +namespace clang { +namespace tidy { +namespace matchers { + +AST_MATCHER(QualType, isExpensiveToCopy) { + return type_traits::isExpensiveToCopy(Node, Finder->getASTContext()); +} + +} // namespace matchers +} // namespace tidy +} // namespace clang Index: clang-tidy/utils/TypeTraits.h =================================================================== --- clang-tidy/utils/TypeTraits.h +++ clang-tidy/utils/TypeTraits.h @@ -0,0 +1,31 @@ +//===--- TypeTraits.h - clang-tidy-------------------------------*- C++ -*-===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// + +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_UTILS_TYPETRAITS_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_UTILS_TYPETRAITS_H + +#include "clang/AST/ASTContext.h" +#include "clang/AST/Type.h" + +namespace clang { +namespace tidy { +namespace type_traits { + +/// Returns true if \c Type is defined has no non-trivial copy-constructor or +/// destructor. +bool classHasTrivialCopyAndDestroy(QualType Type); + +// \brief Returns true If \c Type is expensive to copy. +bool isExpensiveToCopy(QualType Type, ASTContext &Context); + +} // type_traits +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_UTILS_TYPETRAITS_H Index: clang-tidy/utils/TypeTraits.cpp =================================================================== --- clang-tidy/utils/TypeTraits.cpp +++ clang-tidy/utils/TypeTraits.cpp @@ -0,0 +1,38 @@ +//===--- TypeTraits.cpp - clang-tidy---------------------------------------===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// + +#include "TypeTraits.h" +#include "clang/AST/ASTContext.h" +#include "clang/AST/DeclCXX.h" + +namespace clang { +namespace tidy { +namespace type_traits { + +bool classHasTrivialCopyAndDestroy(QualType Type) { + auto *Record = Type->getAsCXXRecordDecl(); + return Record && Record->hasDefinition() && + !Record->hasNonTrivialCopyConstructor() && + !Record->hasNonTrivialDestructor(); +} + +bool isExpensiveToCopy(QualType Type, ASTContext &Context) { + // 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 (Type->isDependentType()) + return false; + // Ignore trivially copyable types. + return !(Type->isScalarType() || Type.isTriviallyCopyableType(Context) || + classHasTrivialCopyAndDestroy(Type)); +} + +} // type_traits +} // namespace tidy +} // namespace clang Index: docs/clang-tidy/checks/misc-move-constructor-init.rst =================================================================== --- docs/clang-tidy/checks/misc-move-constructor-init.rst +++ docs/clang-tidy/checks/misc-move-constructor-init.rst @@ -5,3 +5,6 @@ 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. 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,59 @@ 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 argument 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) {} + NegativeParamTriviallyCopyable(int I) : I_(I) {} + TriviallyCopyable T_; + int I_; +}; + +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_; +};