diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.cpp b/clang-tools-extra/clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.cpp index 6ff60495a72b..b7f5eb6e1e33 100644 --- a/clang-tools-extra/clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.cpp +++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.cpp @@ -25,12 +25,16 @@ SpecialMemberFunctionsCheck::SpecialMemberFunctionsCheck( StringRef Name, ClangTidyContext *Context) : ClangTidyCheck(Name, Context), AllowMissingMoveFunctions(Options.get("AllowMissingMoveFunctions", 0)), - AllowSoleDefaultDtor(Options.get("AllowSoleDefaultDtor", 0)) {} + AllowSoleDefaultDtor(Options.get("AllowSoleDefaultDtor", 0)), + AllowMissingMoveFunctionsWhenCopyIsDeleted( + Options.get("AllowMissingMoveFunctionsWhenCopyIsDeleted", 0)) {} void SpecialMemberFunctionsCheck::storeOptions( ClangTidyOptions::OptionMap &Opts) { Options.store(Opts, "AllowMissingMoveFunctions", AllowMissingMoveFunctions); Options.store(Opts, "AllowSoleDefaultDtor", AllowSoleDefaultDtor); + Options.store(Opts, "AllowMissingMoveFunctionsWhenCopyIsDeleted", + AllowMissingMoveFunctionsWhenCopyIsDeleted); } void SpecialMemberFunctionsCheck::registerMatchers(MatchFinder *Finder) { @@ -103,17 +107,18 @@ void SpecialMemberFunctionsCheck::check( ClassDefId ID(MatchedDecl->getLocation(), std::string(MatchedDecl->getName())); - auto StoreMember = [this, &ID](SpecialMemberFunctionKind Kind) { - llvm::SmallVectorImpl &Members = + auto StoreMember = [this, &ID](SpecialMemberFunctionData data) { + llvm::SmallVectorImpl &Members = ClassWithSpecialMembers[ID]; - if (!llvm::is_contained(Members, Kind)) - Members.push_back(Kind); + if (!llvm::is_contained(Members, data)) + Members.push_back(std::move(data)); }; if (const auto *Dtor = Result.Nodes.getNodeAs("dtor")) { - StoreMember(Dtor->isDefaulted() - ? SpecialMemberFunctionKind::DefaultDestructor - : SpecialMemberFunctionKind::NonDefaultDestructor); + StoreMember({Dtor->isDefaulted() + ? SpecialMemberFunctionKind::DefaultDestructor + : SpecialMemberFunctionKind::NonDefaultDestructor, + Dtor->isDeleted()}); } std::initializer_list> @@ -123,8 +128,9 @@ void SpecialMemberFunctionsCheck::check( {"move-assign", SpecialMemberFunctionKind::MoveAssignment}}; for (const auto &KV : Matchers) - if (Result.Nodes.getNodeAs(KV.first)) { - StoreMember(KV.second); + if (const auto *MethodDecl = + Result.Nodes.getNodeAs(KV.first)) { + StoreMember({KV.second, MethodDecl->isDeleted()}); } } @@ -136,11 +142,19 @@ void SpecialMemberFunctionsCheck::onEndOfTranslationUnit() { void SpecialMemberFunctionsCheck::checkForMissingMembers( const ClassDefId &ID, - llvm::ArrayRef DefinedMembers) { + llvm::ArrayRef DefinedMembers) { llvm::SmallVector MissingMembers; auto HasMember = [&](SpecialMemberFunctionKind Kind) { - return llvm::is_contained(DefinedMembers, Kind); + return llvm::any_of(DefinedMembers, [Kind](const auto &data) { + return data.FunctionKind == Kind; + }); + }; + + auto IsDeleted = [&](SpecialMemberFunctionKind Kind) { + return llvm::any_of(DefinedMembers, [Kind](const auto &data) { + return data.FunctionKind == Kind && data.IsDeleted; + }); }; auto RequireMember = [&](SpecialMemberFunctionKind Kind) { @@ -171,16 +185,23 @@ void SpecialMemberFunctionsCheck::checkForMissingMembers( RequireMember(SpecialMemberFunctionKind::CopyAssignment); } - if (RequireFive) { + if (RequireFive && + !(AllowMissingMoveFunctionsWhenCopyIsDeleted && + (IsDeleted(SpecialMemberFunctionKind::CopyConstructor) && + IsDeleted(SpecialMemberFunctionKind::CopyAssignment)))) { assert(RequireThree); RequireMember(SpecialMemberFunctionKind::MoveConstructor); RequireMember(SpecialMemberFunctionKind::MoveAssignment); } - if (!MissingMembers.empty()) + if (!MissingMembers.empty()) { + llvm::SmallVector DefinedMemberKinds; + llvm::transform(DefinedMembers, std::back_inserter(DefinedMemberKinds), + [](const auto &data) { return data.FunctionKind; }); diag(ID.first, "class '%0' defines %1 but does not define %2") - << ID.second << cppcoreguidelines::join(DefinedMembers, " and ") + << ID.second << cppcoreguidelines::join(DefinedMemberKinds, " and ") << cppcoreguidelines::join(MissingMembers, " or "); + } } } // namespace cppcoreguidelines diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.h b/clang-tools-extra/clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.h index 29982ba4796a..78468cc5c31e 100644 --- a/clang-tools-extra/clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.h +++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.h @@ -43,19 +43,30 @@ public: MoveAssignment }; + struct SpecialMemberFunctionData { + SpecialMemberFunctionKind FunctionKind; + bool IsDeleted; + + bool operator==(const SpecialMemberFunctionData &Other) { + return (Other.FunctionKind == FunctionKind) && + (Other.IsDeleted == IsDeleted); + } + }; + using ClassDefId = std::pair; using ClassDefiningSpecialMembersMap = llvm::DenseMap>; + llvm::SmallVector>; private: void checkForMissingMembers( const ClassDefId &ID, - llvm::ArrayRef DefinedSpecialMembers); + llvm::ArrayRef DefinedSpecialMembers); const bool AllowMissingMoveFunctions; const bool AllowSoleDefaultDtor; + const bool AllowMissingMoveFunctionsWhenCopyIsDeleted; ClassDefiningSpecialMembersMap ClassWithSpecialMembers; }; diff --git a/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-special-member-functions.rst b/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-special-member-functions.rst index 28a5e8e5a0df..4ced6c1bc563 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-special-member-functions.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-special-member-functions.rst @@ -46,4 +46,19 @@ Options A(const A&); A& operator=(const A&); ~A(); - } + }; + +.. option:: AllowMissingMoveFunctionsWhenCopyIsDeleted + + When set to `1` (default is `0`), this check doesn't flag classes which define deleted copy + operations but don't define move operations. This flags is related to Google C++ Style Guide + https://google.github.io/styleguide/cppguide.html#Copyable_Movable_Types. With this option enabled, the + following class won't be flagged: + + .. code-block:: c++ + + struct A { + A(const A&) = delete; + A& operator=(const A&) = delete; + ~A(); + }; diff --git a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-special-member-functions-allow-missing-move-when-copy-is-deleted.cpp b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-special-member-functions-allow-missing-move-when-copy-is-deleted.cpp new file mode 100644 index 000000000000..216a49a44eff --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-special-member-functions-allow-missing-move-when-copy-is-deleted.cpp @@ -0,0 +1,49 @@ +// RUN: %check_clang_tidy %s cppcoreguidelines-special-member-functions %t -- -config="{CheckOptions: [{key: cppcoreguidelines-special-member-functions.AllowMissingMoveFunctionsWhenCopyIsDeleted, value: 1}]}" -- + +class DefinesEverything { + DefinesEverything(const DefinesEverything &); + DefinesEverything(DefinesEverything &&); + DefinesEverything &operator=(const DefinesEverything &); + DefinesEverything &operator=(DefinesEverything &&); + ~DefinesEverything(); +}; + +class DefinesNothing { +}; + +class DeletedCopyCtorAndOperator { + ~DeletedCopyCtorAndOperator() = default; + DeletedCopyCtorAndOperator(const DeletedCopyCtorAndOperator &) = delete; + DeletedCopyCtorAndOperator &operator=(const DeletedCopyCtorAndOperator &) = delete; +}; + +// CHECK-MESSAGES: [[@LINE+1]]:7: warning: class 'DefaultedCopyCtorAndOperator' defines a default destructor, a copy constructor and a copy assignment operator but does not define a move constructor or a move assignment operator [cppcoreguidelines-special-member-functions] +class DefaultedCopyCtorAndOperator { + ~DefaultedCopyCtorAndOperator() = default; + DefaultedCopyCtorAndOperator(const DefaultedCopyCtorAndOperator &) = default; + DefaultedCopyCtorAndOperator &operator=(const DefaultedCopyCtorAndOperator &) = default; +}; + +// CHECK-MESSAGES: [[@LINE+1]]:7: warning: class 'DefinedCopyCtorAndOperator' defines a default destructor, a copy constructor and a copy assignment operator but does not define a move constructor or a move assignment operator [cppcoreguidelines-special-member-functions] +class DefinedCopyCtorAndOperator { + ~DefinedCopyCtorAndOperator() = default; + DefinedCopyCtorAndOperator(const DefinedCopyCtorAndOperator &); + DefinedCopyCtorAndOperator &operator=(const DefinedCopyCtorAndOperator &); +}; + +// CHECK-MESSAGES: [[@LINE+1]]:7: warning: class 'MissingCopyCtor' defines a default destructor and a copy assignment operator but does not define a copy constructor, a move constructor or a move assignment operator [cppcoreguidelines-special-member-functions] +class MissingCopyCtor { + ~MissingCopyCtor() = default; + MissingCopyCtor &operator=(const MissingCopyCtor &) = delete; +}; + +// CHECK-MESSAGES: [[@LINE+1]]:7: warning: class 'MissingCopyOperator' defines a default destructor and a copy constructor but does not define a copy assignment operator, a move constructor or a move assignment operator [cppcoreguidelines-special-member-functions] +class MissingCopyOperator { + ~MissingCopyOperator() = default; + MissingCopyOperator(const MissingCopyOperator &) = delete; +}; + +// CHECK-MESSAGES: [[@LINE+1]]:7: warning: class 'MissingAll' defines a default destructor but does not define a copy constructor, a copy assignment operator, a move constructor or a move assignment operator [cppcoreguidelines-special-member-functions] +class MissingAll { + ~MissingAll() = default; +};