Index: clang-tidy/cppcoreguidelines/CMakeLists.txt =================================================================== --- clang-tidy/cppcoreguidelines/CMakeLists.txt +++ clang-tidy/cppcoreguidelines/CMakeLists.txt @@ -13,6 +13,7 @@ ProTypeStaticCastDowncastCheck.cpp ProTypeUnionAccessCheck.cpp ProTypeVarargCheck.cpp + SpecialMemberFunctionsCheck.cpp SlicingCheck.cpp LINK_LIBS Index: clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp =================================================================== --- clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp +++ clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp @@ -22,6 +22,7 @@ #include "ProTypeStaticCastDowncastCheck.h" #include "ProTypeUnionAccessCheck.h" #include "ProTypeVarargCheck.h" +#include "SpecialMemberFunctionsCheck.h" #include "SlicingCheck.h" namespace clang { @@ -54,6 +55,8 @@ "cppcoreguidelines-pro-type-union-access"); CheckFactories.registerCheck( "cppcoreguidelines-pro-type-vararg"); + CheckFactories.registerCheck( + "cppcoreguidelines-special-member-functions"); CheckFactories.registerCheck( "cppcoreguidelines-slicing"); CheckFactories.registerCheck( Index: clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.h =================================================================== --- /dev/null +++ clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.h @@ -0,0 +1,101 @@ +//===--- SpecialMemberFunctionsCheck.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_CPPCOREGUIDELINES_SPECIAL_MEMBER_FUNCTIONS_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_SPECIAL_MEMBER_FUNCTIONS_H + +#include "../ClangTidy.h" + +#include "llvm/ADT/DenseMapInfo.h" + +namespace clang { +namespace tidy { +namespace cppcoreguidelines { + +/// Checks for classes where some, but not all, of the special member functions +/// are defined. +/// +/// For the user-facing documentation see: +/// 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) {} + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + void onEndOfTranslationUnit() override; + + enum class SpecialMemberFunctionKind { + Destructor, + CopyConstructor, + CopyAssignment, + MoveConstructor, + MoveAssignment + }; + + using ClassDefId = std::pair; + + using ClassDefiningSpecialMembersMap = llvm::DenseMap>; + +private: + + static llvm::StringRef toString(SpecialMemberFunctionKind K); + + static std::string join(llvm::ArrayRef SMFS, + llvm::StringRef AndOr); + + ClassDefiningSpecialMembersMap ClassWithSpecialMembers; +}; + +} // namespace cppcoreguidelines +} // namespace tidy +} // namespace clang + +namespace llvm { +/// Specialisation of DenseMapInfo to allow ClassDefId objects in DenseMaps +/// FIXME: Move this to the corresponding cpp file as is done for +/// clang-tidy/readability/IdentifierNamingCheck.cpp. +template <> +struct DenseMapInfo< + clang::tidy::cppcoreguidelines::SpecialMemberFunctionsCheck::ClassDefId> { + using ClassDefId = + clang::tidy::cppcoreguidelines::SpecialMemberFunctionsCheck::ClassDefId; + + static inline ClassDefId getEmptyKey() { + return ClassDefId( + clang::SourceLocation::getFromRawEncoding(static_cast(-1)), + "EMPTY"); + } + + static inline ClassDefId getTombstoneKey() { + return ClassDefId( + clang::SourceLocation::getFromRawEncoding(static_cast(-2)), + "TOMBSTONE"); + } + + static unsigned getHashValue(ClassDefId Val) { + assert(Val != getEmptyKey() && "Cannot hash the empty key!"); + assert(Val != getTombstoneKey() && "Cannot hash the tombstone key!"); + + std::hash SecondHash; + return Val.first.getRawEncoding() + SecondHash(Val.second); + } + + static bool isEqual(ClassDefId LHS, ClassDefId RHS) { + if (RHS == getEmptyKey()) + return LHS == getEmptyKey(); + if (RHS == getTombstoneKey()) + return LHS == getTombstoneKey(); + return LHS == RHS; + } +}; + +} // namespace llvm + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_SPECIAL_MEMBER_FUNCTIONS_H Index: clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.cpp =================================================================== --- /dev/null +++ clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.cpp @@ -0,0 +1,132 @@ +//===--- SpecialMemberFunctionsCheck.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 "SpecialMemberFunctionsCheck.h" + +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include "llvm/ADT/DenseMapInfo.h" +#include "llvm/ADT/StringExtras.h" + +#define DEBUG_TYPE "clang-tidy" + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { +namespace cppcoreguidelines { + +void SpecialMemberFunctionsCheck::registerMatchers(MatchFinder *Finder) { + if (!getLangOpts().CPlusPlus) + return; + Finder->addMatcher( + cxxRecordDecl( + eachOf( + has(cxxDestructorDecl(unless(isImplicit())).bind("dtor")), + has(cxxConstructorDecl(isCopyConstructor(), unless(isImplicit())) + .bind("copy-ctor")), + has(cxxMethodDecl(isCopyAssignmentOperator(), + unless(isImplicit())) + .bind("copy-assign")), + has(cxxConstructorDecl(isMoveConstructor(), unless(isImplicit())) + .bind("move-ctor")), + has(cxxMethodDecl(isMoveAssignmentOperator(), + unless(isImplicit())) + .bind("move-assign")))) + .bind("class-def"), + this); +} + +llvm::StringRef SpecialMemberFunctionsCheck::toString( + SpecialMemberFunctionsCheck::SpecialMemberFunctionKind K) { + switch (K) { + case SpecialMemberFunctionKind::Destructor: + return "a destructor"; + case SpecialMemberFunctionKind::CopyConstructor: + return "a copy constructor"; + case SpecialMemberFunctionKind::CopyAssignment: + return "a copy assignment operator"; + case SpecialMemberFunctionKind::MoveConstructor: + return "a move constructor"; + case SpecialMemberFunctionKind::MoveAssignment: + return "a move assignment operator"; + } +} + +std::string SpecialMemberFunctionsCheck::join( + llvm::ArrayRef SMFS, llvm::StringRef AndOr) { + + assert(!SMFS.empty() && + "List of defined or undefined members should never be empty."); + std::string Buffer; + llvm::raw_string_ostream Stream(Buffer); + + Stream << toString(SMFS[0]); + size_t LastIndex = SMFS.size() - 1; + for (size_t i = 1; i < LastIndex; ++i) { + Stream << ", " << toString(SMFS[i]); + } + if (LastIndex != 0) { + Stream << AndOr << toString(SMFS[LastIndex]); + } + return Stream.str(); +} + +void SpecialMemberFunctionsCheck::check( + const MatchFinder::MatchResult &Result) { + const CXXRecordDecl *MatchedDecl = + Result.Nodes.getNodeAs("class-def"); + if (!MatchedDecl) + return; + + ClassDefId ID(MatchedDecl->getLocation(), MatchedDecl->getName()); + + std::initializer_list> + Matchers = {{"dtor", SpecialMemberFunctionKind::Destructor}, + {"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)) + ClassWithSpecialMembers[ID].push_back(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) { + ArrayRef DefinedSpecialMembers = C.second; + + if (DefinedSpecialMembers.size() == AllSpecialMembers.size()) + continue; + + 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 "); + } +} +} // namespace cppcoreguidelines +} // namespace tidy +} // namespace clang Index: docs/ReleaseNotes.rst =================================================================== --- docs/ReleaseNotes.rst +++ docs/ReleaseNotes.rst @@ -64,6 +64,11 @@ Flags slicing of member variables or vtable. +- New `cppcoreguidelines-special-member-functions + `_ check + + Flags classes where some, but not all, special member functions are user-defined. + Improvements to include-fixer ----------------------------- Index: docs/clang-tidy/checks/cppcoreguidelines-special-member-functions.rst =================================================================== --- /dev/null +++ docs/clang-tidy/checks/cppcoreguidelines-special-member-functions.rst @@ -0,0 +1,21 @@ +.. title:: clang-tidy - cppcoreguidelines-special-member-functions + +cppcoreguidelines-special-member-functions +========================================== + +The check finds classes where some but not all of the special member functions +are defined. + +By default the compiler defines a copy constructor, copy assignment operator, +move constructor, move assignment operator and destructor. The default can be +supressed by explicit user-definitions. The relationship between which +functions will be supressed by definitions of other functions is complicated +and it is advised that all five are defaulted or explicitly defined. + +Note that defining a function with ``= delete`` is considered to be a +definition. + +This rule is part of the "Constructors, assignments, and destructors" profile of the C++ Core +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. Index: docs/clang-tidy/checks/list.rst =================================================================== --- docs/clang-tidy/checks/list.rst +++ docs/clang-tidy/checks/list.rst @@ -30,6 +30,7 @@ cppcoreguidelines-pro-type-union-access cppcoreguidelines-pro-type-vararg cppcoreguidelines-slicing + cppcoreguidelines-special-member-functions google-build-explicit-make-pair google-build-namespaces google-build-using-namespace Index: test/clang-tidy/cppcoreguidelines-special-member-functions-cxx-03.cpp =================================================================== --- /dev/null +++ test/clang-tidy/cppcoreguidelines-special-member-functions-cxx-03.cpp @@ -0,0 +1,26 @@ +// RUN: %check_clang_tidy %s cppcoreguidelines-special-member-functions %t -- -- -std=c++03 + +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] + +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 DefinesNothing { +}; + +class DefinesEverything { + DefinesEverything(const DefinesEverything &); + DefinesEverything &operator=(const DefinesEverything &); + ~DefinesEverything(); +}; + Index: test/clang-tidy/cppcoreguidelines-special-member-functions.cpp =================================================================== --- /dev/null +++ test/clang-tidy/cppcoreguidelines-special-member-functions.cpp @@ -0,0 +1,52 @@ +// RUN: %check_clang_tidy %s cppcoreguidelines-special-member-functions %t + +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] + +class DefinesCopyConstructor { + DefinesCopyConstructor(const DefinesCopyConstructor &); +}; +// CHECK-MESSAGES: [[@LINE-3]]:7: warning: class 'DefinesCopyConstructor' defines a copy constructor but does not define a destructor, a copy assignment operator, a move constructor or a move 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, a copy constructor, a move constructor or a move assignment operator [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; +};