Index: clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.h =================================================================== --- clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.h +++ clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.h @@ -25,14 +25,16 @@ /// http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-special-member-functions.html class SpecialMemberFunctionsCheck : public ClangTidyCheck { public: - SpecialMemberFunctionsCheck(StringRef Name, ClangTidyContext *Context) - : ClangTidyCheck(Name, Context) {} + SpecialMemberFunctionsCheck(StringRef Name, ClangTidyContext *Context); + void storeOptions(ClangTidyOptions::OptionMap &Opts) override; void registerMatchers(ast_matchers::MatchFinder *Finder) override; void check(const ast_matchers::MatchFinder::MatchResult &Result) override; void onEndOfTranslationUnit() override; enum class SpecialMemberFunctionKind : uint8_t { Destructor, + DefaultDestructor, + NonDefaultDestructor, CopyConstructor, CopyAssignment, MoveConstructor, @@ -46,6 +48,11 @@ llvm::SmallVector>; private: + void checkForMissingMembers(const ClassDefId &ID, + llvm::ArrayRef DefinedSpecialMembers); + + const bool AllowMissingMoveFunctions; + const bool AllowSoleDefaultDtor; ClassDefiningSpecialMembersMap ClassWithSpecialMembers; }; Index: clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.cpp =================================================================== --- clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.cpp +++ clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.cpp @@ -22,6 +22,18 @@ namespace tidy { namespace cppcoreguidelines { +SpecialMemberFunctionsCheck::SpecialMemberFunctionsCheck( + StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context), + AllowMissingMoveFunctions(Options.get("AllowMissingMoveFunctions", 0)), + AllowSoleDefaultDtor(Options.get("AllowSoleDefaultDtor", 0)) {} + +void SpecialMemberFunctionsCheck::storeOptions( + ClangTidyOptions::OptionMap &Opts) { + Options.store(Opts, "AllowMissingMoveFunctions", AllowMissingMoveFunctions); + Options.store(Opts, "AllowSoleDefaultDtor", AllowSoleDefaultDtor); +} + void SpecialMemberFunctionsCheck::registerMatchers(MatchFinder *Finder) { if (!getLangOpts().CPlusPlus) return; @@ -48,6 +60,10 @@ switch (K) { case SpecialMemberFunctionsCheck::SpecialMemberFunctionKind::Destructor: return "a destructor"; + case SpecialMemberFunctionsCheck::SpecialMemberFunctionKind::DefaultDestructor: + return "a default destructor"; + case SpecialMemberFunctionsCheck::SpecialMemberFunctionKind::NonDefaultDestructor: + return "a non-default destructor"; case SpecialMemberFunctionsCheck::SpecialMemberFunctionKind::CopyConstructor: return "a copy constructor"; case SpecialMemberFunctionsCheck::SpecialMemberFunctionKind::CopyAssignment: @@ -88,51 +104,86 @@ ClassDefId ID(MatchedDecl->getLocation(), MatchedDecl->getName()); + auto StoreMember = [this, &ID](SpecialMemberFunctionKind Kind) { + llvm::SmallVectorImpl &Members = + ClassWithSpecialMembers[ID]; + if (!llvm::is_contained(Members, Kind)) + Members.push_back(Kind); + }; + + if (auto Dtor = Result.Nodes.getNodeAs("dtor")) { + StoreMember(Dtor->isDefaulted() + ? SpecialMemberFunctionKind::DefaultDestructor + : SpecialMemberFunctionKind::NonDefaultDestructor); + } + std::initializer_list> - Matchers = {{"dtor", SpecialMemberFunctionKind::Destructor}, - {"copy-ctor", SpecialMemberFunctionKind::CopyConstructor}, + Matchers = {{"copy-ctor", SpecialMemberFunctionKind::CopyConstructor}, {"copy-assign", SpecialMemberFunctionKind::CopyAssignment}, {"move-ctor", SpecialMemberFunctionKind::MoveConstructor}, {"move-assign", SpecialMemberFunctionKind::MoveAssignment}}; for (const auto &KV : Matchers) if (Result.Nodes.getNodeAs(KV.first)) { - SpecialMemberFunctionKind Kind = KV.second; - llvm::SmallVectorImpl &Members = - ClassWithSpecialMembers[ID]; - if (find(Members, Kind) == Members.end()) - Members.push_back(Kind); + StoreMember(KV.second); } } void SpecialMemberFunctionsCheck::onEndOfTranslationUnit() { - llvm::SmallVector AllSpecialMembers = { - SpecialMemberFunctionKind::Destructor, - SpecialMemberFunctionKind::CopyConstructor, - SpecialMemberFunctionKind::CopyAssignment}; - - if (getLangOpts().CPlusPlus11) { - AllSpecialMembers.push_back(SpecialMemberFunctionKind::MoveConstructor); - AllSpecialMembers.push_back(SpecialMemberFunctionKind::MoveAssignment); + for (const auto &C : ClassWithSpecialMembers) { + checkForMissingMembers(C.first, C.second); } +} - for (const auto &C : ClassWithSpecialMembers) { - const auto &DefinedSpecialMembers = C.second; +void SpecialMemberFunctionsCheck::checkForMissingMembers(const ClassDefId &ID, + llvm::ArrayRef DefinedMembers) { + llvm::SmallVector MissingMembers; + + auto HasMember = [&](SpecialMemberFunctionKind Kind) { + return llvm::is_contained(DefinedMembers, Kind); + }; + + auto RequireMember = [&](SpecialMemberFunctionKind Kind) { + if (!HasMember(Kind)) + MissingMembers.push_back(Kind); + }; + + bool RequireThree = + HasMember(SpecialMemberFunctionKind::NonDefaultDestructor) || + (!AllowSoleDefaultDtor && + HasMember(SpecialMemberFunctionKind::DefaultDestructor)) || + HasMember(SpecialMemberFunctionKind::CopyConstructor) || + HasMember(SpecialMemberFunctionKind::CopyAssignment) || + HasMember(SpecialMemberFunctionKind::MoveConstructor) || + HasMember(SpecialMemberFunctionKind::MoveAssignment); + + bool RequireFive = + (!AllowMissingMoveFunctions && RequireThree && + getLangOpts().CPlusPlus11) || + HasMember(SpecialMemberFunctionKind::MoveConstructor) || + HasMember(SpecialMemberFunctionKind::MoveAssignment); + + if (RequireThree) { + if (!HasMember(SpecialMemberFunctionKind::DefaultDestructor) && + !HasMember(SpecialMemberFunctionKind::NonDefaultDestructor)) + MissingMembers.push_back(SpecialMemberFunctionKind::Destructor); - if (DefinedSpecialMembers.size() == AllSpecialMembers.size()) - continue; + RequireMember(SpecialMemberFunctionKind::CopyConstructor); + RequireMember(SpecialMemberFunctionKind::CopyAssignment); + } - llvm::SmallVector UndefinedSpecialMembers; - std::set_difference(AllSpecialMembers.begin(), AllSpecialMembers.end(), - DefinedSpecialMembers.begin(), - DefinedSpecialMembers.end(), - std::back_inserter(UndefinedSpecialMembers)); - - diag(C.first.first, "class '%0' defines %1 but does not define %2") - << C.first.second << join(DefinedSpecialMembers, " and ") - << join(UndefinedSpecialMembers, " or "); + if (RequireFive) { + assert(RequireThree); + RequireMember(SpecialMemberFunctionKind::MoveConstructor); + RequireMember(SpecialMemberFunctionKind::MoveAssignment); } + + if (!MissingMembers.empty()) + diag(ID.first, "class '%0' defines %1 but does not define %2") + << ID.second << join(DefinedMembers, " and ") + << join(MissingMembers, " or "); } + } // namespace cppcoreguidelines } // namespace tidy } // namespace clang Index: docs/clang-tidy/checks/cppcoreguidelines-special-member-functions.rst =================================================================== --- docs/clang-tidy/checks/cppcoreguidelines-special-member-functions.rst +++ docs/clang-tidy/checks/cppcoreguidelines-special-member-functions.rst @@ -19,3 +19,17 @@ Guidelines, corresponding to rule C.21. See https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#c21-if-you-define-or-delete-any-default-operation-define-or-delete-them-all. + +Options +------- + +.. option:: AllowSoleDefaultDtor + + When set to `1`, this check doesn't flag classes with a sole, explicitly + defaulted destructor. Default value is `0`. + +.. option:: AllowMissingMoveFunctions + + When set to `1`, this check doesn't flag classes which define no move + operations at all. It still flags classes which define only one of either + move constructor or move assignment operator. Defaut value is `0`. Index: test/clang-tidy/cppcoreguidelines-special-member-functions-cxx-03.cpp =================================================================== --- test/clang-tidy/cppcoreguidelines-special-member-functions-cxx-03.cpp +++ test/clang-tidy/cppcoreguidelines-special-member-functions-cxx-03.cpp @@ -3,7 +3,7 @@ class DefinesDestructor { ~DefinesDestructor(); }; -// CHECK-MESSAGES: [[@LINE-3]]:7: warning: class 'DefinesDestructor' defines a destructor but does not define a copy constructor or a copy assignment operator [cppcoreguidelines-special-member-functions] +// CHECK-MESSAGES: [[@LINE-3]]:7: warning: class 'DefinesDestructor' defines a non-default destructor but does not define a copy constructor or a copy assignment operator [cppcoreguidelines-special-member-functions] class DefinesCopyConstructor { DefinesCopyConstructor(const DefinesCopyConstructor &); Index: test/clang-tidy/cppcoreguidelines-special-member-functions-relaxed.cpp =================================================================== --- test/clang-tidy/cppcoreguidelines-special-member-functions-relaxed.cpp +++ test/clang-tidy/cppcoreguidelines-special-member-functions-relaxed.cpp @@ -0,0 +1,71 @@ +// RUN: %check_clang_tidy %s cppcoreguidelines-special-member-functions %t -- -config="{CheckOptions: [{key: cppcoreguidelines-special-member-functions.AllowMissingMoveFunctions, value: 1}, {key: cppcoreguidelines-special-member-functions.AllowSoleDefaultDtor, value: 1}]}" -- + +class DefinesDestructor { + ~DefinesDestructor(); +}; +// CHECK-MESSAGES: [[@LINE-3]]:7: warning: class 'DefinesDestructor' defines a non-default destructor but does not define a copy constructor or a copy assignment operator [cppcoreguidelines-special-member-functions] + +class DefinesDefaultedDestructor { + ~DefinesDefaultedDestructor() = default; +}; + +class DefinesCopyConstructor { + DefinesCopyConstructor(const DefinesCopyConstructor &); +}; +// CHECK-MESSAGES: [[@LINE-3]]:7: warning: class 'DefinesCopyConstructor' defines a copy constructor but does not define a destructor or a copy assignment operator [cppcoreguidelines-special-member-functions] + +class DefinesCopyAssignment { + DefinesCopyAssignment &operator=(const DefinesCopyAssignment &); +}; +// CHECK-MESSAGES: [[@LINE-3]]:7: warning: class 'DefinesCopyAssignment' defines a copy assignment operator but does not define a destructor or a copy constructor [cppcoreguidelines-special-member-functions] + +class DefinesMoveConstructor { + DefinesMoveConstructor(DefinesMoveConstructor &&); +}; +// CHECK-MESSAGES: [[@LINE-3]]:7: warning: class 'DefinesMoveConstructor' defines a move constructor but does not define a destructor, a copy constructor, a copy assignment operator or a move assignment operator [cppcoreguidelines-special-member-functions] + +class DefinesMoveAssignment { + DefinesMoveAssignment &operator=(DefinesMoveAssignment &&); +}; +// CHECK-MESSAGES: [[@LINE-3]]:7: warning: class 'DefinesMoveAssignment' defines a move assignment operator but does not define a destructor, a copy constructor, a copy assignment operator or a move constructor [cppcoreguidelines-special-member-functions] +class DefinesNothing { +}; + +class DefinesEverything { + DefinesEverything(const DefinesEverything &); + DefinesEverything &operator=(const DefinesEverything &); + DefinesEverything(DefinesEverything &&); + DefinesEverything &operator=(DefinesEverything &&); + ~DefinesEverything(); +}; + +class DeletesEverything { + DeletesEverything(const DeletesEverything &) = delete; + DeletesEverything &operator=(const DeletesEverything &) = delete; + DeletesEverything(DeletesEverything &&) = delete; + DeletesEverything &operator=(DeletesEverything &&) = delete; + ~DeletesEverything() = delete; +}; + +class DeletesCopyDefaultsMove { + DeletesCopyDefaultsMove(const DeletesCopyDefaultsMove &) = delete; + DeletesCopyDefaultsMove &operator=(const DeletesCopyDefaultsMove &) = delete; + DeletesCopyDefaultsMove(DeletesCopyDefaultsMove &&) = default; + DeletesCopyDefaultsMove &operator=(DeletesCopyDefaultsMove &&) = default; + ~DeletesCopyDefaultsMove() = default; +}; + +template +struct TemplateClass { + TemplateClass() = default; + TemplateClass(const TemplateClass &); + TemplateClass &operator=(const TemplateClass &); + TemplateClass(TemplateClass &&); + TemplateClass &operator=(TemplateClass &&); + ~TemplateClass(); +}; + +// Multiple instantiations of a class template will trigger multiple matches for defined special members. +// This should not cause problems. +TemplateClass InstantiationWithInt; +TemplateClass InstantiationWithDouble; Index: test/clang-tidy/cppcoreguidelines-special-member-functions.cpp =================================================================== --- test/clang-tidy/cppcoreguidelines-special-member-functions.cpp +++ test/clang-tidy/cppcoreguidelines-special-member-functions.cpp @@ -3,7 +3,12 @@ class DefinesDestructor { ~DefinesDestructor(); }; -// CHECK-MESSAGES: [[@LINE-3]]:7: warning: class 'DefinesDestructor' defines a destructor but does not define a copy constructor, a copy assignment operator, a move constructor or a move assignment operator [cppcoreguidelines-special-member-functions] +// CHECK-MESSAGES: [[@LINE-3]]:7: warning: class 'DefinesDestructor' defines a non-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 DefinesDefaultedDestructor { + ~DefinesDefaultedDestructor() = default; +}; +// CHECK-MESSAGES: [[@LINE-3]]:7: warning: class 'DefinesDefaultedDestructor' 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 DefinesCopyConstructor { DefinesCopyConstructor(const DefinesCopyConstructor &);