https://github.com/localspook created https://github.com/llvm/llvm-project/pull/172255
None >From c1d870a8219554f8a31af9e79f1034b2107f94a6 Mon Sep 17 00:00:00 2001 From: Victor Chernyakin <[email protected]> Date: Sun, 14 Dec 2025 21:50:56 -0800 Subject: [PATCH] [clang-tidy][NFC] Refactor `cppcoreguidelines-special-member-functions` --- .../SpecialMemberFunctionsCheck.cpp | 286 ++++++------------ .../SpecialMemberFunctionsCheck.h | 77 +---- 2 files changed, 99 insertions(+), 264 deletions(-) diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.cpp b/clang-tools-extra/clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.cpp index 851392f5ba8a2..09204bc82e70f 100644 --- a/clang-tools-extra/clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.cpp +++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.cpp @@ -7,23 +7,43 @@ //===----------------------------------------------------------------------===// #include "SpecialMemberFunctionsCheck.h" - -#include "clang/AST/ASTContext.h" -#include "clang/ASTMatchers/ASTMatchFinder.h" -#include "llvm/ADT/StringExtras.h" - -#define DEBUG_TYPE "clang-tidy" +#include "clang/ASTMatchers/ASTMatchers.h" using namespace clang::ast_matchers; namespace clang::tidy::cppcoreguidelines { namespace { -AST_MATCHER(CXXRecordDecl, isInMacro) { - return Node.getBeginLoc().isMacroID() && Node.getEndLoc().isMacroID(); -} + +enum SpecialMemberFunctions : uint8_t { + None = 0, + Dtor = 1 << 0, + DefaultDtor = 1 << 1, + NonDefaultDtor = 1 << 2, + CopyCtor = 1 << 3, + CopyAssignment = 1 << 4, + CopyOps = CopyCtor | CopyAssignment, + MoveCtor = 1 << 5, + MoveAssignment = 1 << 6, + MoveOps = MoveCtor | MoveAssignment, + LLVM_MARK_AS_BITMASK_ENUM(MoveAssignment), +}; + } // namespace +static StringRef toString(size_t K) { + static constexpr StringRef EnumToStringMap[] = { + "a destructor", + "a default destructor", + "a non-default destructor", + "a copy constructor", + "a copy assignment operator", + "a move constructor", + "a move assignment operator", + }; + return EnumToStringMap[K]; +} + SpecialMemberFunctionsCheck::SpecialMemberFunctionsCheck( StringRef Name, ClangTidyContext *Context) : ClangTidyCheck(Name, Context), AllowMissingMoveFunctions(Options.get( @@ -46,206 +66,92 @@ void SpecialMemberFunctionsCheck::storeOptions( Options.store(Opts, "IgnoreMacros", IgnoreMacros); } -std::optional<TraversalKind> -SpecialMemberFunctionsCheck::getCheckTraversalKind() const { - return AllowImplicitlyDeletedCopyOrMove ? TK_AsIs - : TK_IgnoreUnlessSpelledInSource; -} - -void SpecialMemberFunctionsCheck::registerMatchers(MatchFinder *Finder) { - const auto IsNotImplicitOrDeleted = anyOf(unless(isImplicit()), isDeleted()); - const ast_matchers::internal::Matcher<CXXRecordDecl> Anything = anything(); - - Finder->addMatcher( - cxxRecordDecl( - unless(isImplicit()), IgnoreMacros ? unless(isInMacro()) : Anything, - eachOf(has(cxxDestructorDecl(unless(isImplicit())).bind("dtor")), - has(cxxConstructorDecl(isCopyConstructor(), - IsNotImplicitOrDeleted) - .bind("copy-ctor")), - has(cxxMethodDecl(isCopyAssignmentOperator(), - IsNotImplicitOrDeleted) - .bind("copy-assign")), - has(cxxConstructorDecl(isMoveConstructor(), - IsNotImplicitOrDeleted) - .bind("move-ctor")), - has(cxxMethodDecl(isMoveAssignmentOperator(), - IsNotImplicitOrDeleted) - .bind("move-assign")))) - .bind("class-def"), - this); -} - -static llvm::StringRef -toString(SpecialMemberFunctionsCheck::SpecialMemberFunctionKind K) { - 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: - return "a copy assignment operator"; - case SpecialMemberFunctionsCheck::SpecialMemberFunctionKind::MoveConstructor: - return "a move constructor"; - case SpecialMemberFunctionsCheck::SpecialMemberFunctionKind::MoveAssignment: - return "a move assignment operator"; +static std::string joinSMFs(SpecialMemberFunctions SMFs, StringRef AndOr) { + assert(SMFs && "List of defined or undefined members should never be empty."); + std::string Buffer; + const size_t TotalSMFs = llvm::popcount(llvm::to_underlying(SMFs)); + for (size_t SMFsLeft = TotalSMFs, I = 0; SMFsLeft > 0; ++I) { + if (!(SMFs & (1 << I))) + continue; + if (SMFsLeft != TotalSMFs) + Buffer += SMFsLeft == 1 ? AndOr : ", "; + Buffer += toString(I); + --SMFsLeft; } - llvm_unreachable("Unhandled SpecialMemberFunctionKind"); + return Buffer; } -static std::string -join(ArrayRef<SpecialMemberFunctionsCheck::SpecialMemberFunctionKind> 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]); - const 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::registerMatchers(MatchFinder *Finder) { + Finder->addMatcher(cxxRecordDecl().bind("decl"), this); } void SpecialMemberFunctionsCheck::check( const MatchFinder::MatchResult &Result) { - const auto *MatchedDecl = Result.Nodes.getNodeAs<CXXRecordDecl>("class-def"); - if (!MatchedDecl) + const auto &Class = *Result.Nodes.getNodeAs<CXXRecordDecl>("decl"); + if (IgnoreMacros && Class.getBeginLoc().isMacroID() && + Class.getEndLoc().isMacroID()) return; - ClassDefId ID(MatchedDecl->getLocation(), - std::string(MatchedDecl->getName())); - - auto StoreMember = [this, &ID](SpecialMemberFunctionData Data) { - llvm::SmallVectorImpl<SpecialMemberFunctionData> &Members = - ClassWithSpecialMembers[ID]; - if (!llvm::is_contained(Members, Data)) - Members.push_back(std::move(Data)); - }; - - if (const auto *Dtor = Result.Nodes.getNodeAs<CXXMethodDecl>("dtor")) { - SpecialMemberFunctionKind DestructorType = - SpecialMemberFunctionKind::Destructor; - if (Dtor->isDefined()) { - DestructorType = Dtor->getDefinition()->isDefaulted() - ? SpecialMemberFunctionKind::DefaultDestructor - : SpecialMemberFunctionKind::NonDefaultDestructor; - } - StoreMember({DestructorType, Dtor->isDeleted()}); - } - - const std::initializer_list<std::pair<std::string, SpecialMemberFunctionKind>> - Matchers = {{"copy-ctor", SpecialMemberFunctionKind::CopyConstructor}, - {"copy-assign", SpecialMemberFunctionKind::CopyAssignment}, - {"move-ctor", SpecialMemberFunctionKind::MoveConstructor}, - {"move-assign", SpecialMemberFunctionKind::MoveAssignment}}; - - for (const auto &KV : Matchers) - if (const auto *MethodDecl = - Result.Nodes.getNodeAs<CXXMethodDecl>(KV.first)) { - StoreMember( - {KV.second, MethodDecl->isDeleted(), MethodDecl->isImplicit()}); + SpecialMemberFunctions DefinedSMFs{}; + SpecialMemberFunctions ImplicitSMFs{}; + SpecialMemberFunctions DeletedSMFs{}; + for (const CXXMethodDecl *Method : Class.methods()) { + SpecialMemberFunctions NewSMF{}; + if (Method->isCopyAssignmentOperator()) { + NewSMF = CopyAssignment; + } else if (Method->isMoveAssignmentOperator()) { + NewSMF = MoveAssignment; + } else if (const auto *Destructor = dyn_cast<CXXDestructorDecl>(Method)) { + if (!Destructor->isDefined()) + NewSMF = Dtor; + else if (Destructor->getDefinition()->isDefaulted()) + NewSMF = DefaultDtor; + else + NewSMF = NonDefaultDtor; + } else if (const auto *Constructor = dyn_cast<CXXConstructorDecl>(Method)) { + if (Constructor->isCopyConstructor()) + NewSMF = CopyCtor; + else if (Constructor->isMoveConstructor()) + NewSMF = MoveCtor; } -} -void SpecialMemberFunctionsCheck::onEndOfTranslationUnit() { - for (const auto &C : ClassWithSpecialMembers) { - checkForMissingMembers(C.first, C.second); + if (Method->isImplicit()) + ImplicitSMFs |= NewSMF; + else + DefinedSMFs |= NewSMF; + if (Method->isDeleted()) + DeletedSMFs |= NewSMF; } -} -void SpecialMemberFunctionsCheck::checkForMissingMembers( - const ClassDefId &ID, - llvm::ArrayRef<SpecialMemberFunctionData> DefinedMembers) { - llvm::SmallVector<SpecialMemberFunctionKind, 5> MissingMembers; + if (!DefinedSMFs) + return; // Class follows rule of 0. - auto HasMember = [&](SpecialMemberFunctionKind Kind) { - return llvm::any_of(DefinedMembers, [Kind](const auto &Data) { - return Data.FunctionKind == Kind && !Data.IsImplicit; - }); - }; - - auto HasImplicitDeletedMember = [&](SpecialMemberFunctionKind Kind) { - return llvm::any_of(DefinedMembers, [Kind](const auto &Data) { - return Data.FunctionKind == Kind && Data.IsImplicit && Data.IsDeleted; - }); - }; - - auto IsDeleted = [&](SpecialMemberFunctionKind Kind) { - return llvm::any_of(DefinedMembers, [Kind](const auto &Data) { - return Data.FunctionKind == Kind && Data.IsDeleted; - }); - }; - - auto RequireMembers = [&](SpecialMemberFunctionKind Kind1, - SpecialMemberFunctionKind Kind2) { - if (AllowImplicitlyDeletedCopyOrMove && HasImplicitDeletedMember(Kind1) && - HasImplicitDeletedMember(Kind2)) - return; + if (AllowSoleDefaultDtor && !(DefinedSMFs & ~(Dtor | DefaultDtor))) + return; - if (!HasMember(Kind1)) - MissingMembers.push_back(Kind1); + SpecialMemberFunctions RequiredSMFs{}; + if (!(AllowImplicitlyDeletedCopyOrMove && + (ImplicitSMFs & DeletedSMFs & CopyOps) == CopyOps)) + RequiredSMFs |= CopyOps; - if (!HasMember(Kind2)) - MissingMembers.push_back(Kind2); - }; - - const bool RequireThree = - HasMember(SpecialMemberFunctionKind::NonDefaultDestructor) || - (!AllowSoleDefaultDtor && - (HasMember(SpecialMemberFunctionKind::Destructor) || - HasMember(SpecialMemberFunctionKind::DefaultDestructor))) || - HasMember(SpecialMemberFunctionKind::CopyConstructor) || - HasMember(SpecialMemberFunctionKind::CopyAssignment) || - HasMember(SpecialMemberFunctionKind::MoveConstructor) || - HasMember(SpecialMemberFunctionKind::MoveAssignment); - - const bool RequireFive = - (!AllowMissingMoveFunctions && RequireThree && - getLangOpts().CPlusPlus11) || - HasMember(SpecialMemberFunctionKind::MoveConstructor) || - HasMember(SpecialMemberFunctionKind::MoveAssignment); - - if (RequireThree) { - if (!HasMember(SpecialMemberFunctionKind::Destructor) && - !HasMember(SpecialMemberFunctionKind::DefaultDestructor) && - !HasMember(SpecialMemberFunctionKind::NonDefaultDestructor)) - MissingMembers.push_back(SpecialMemberFunctionKind::Destructor); - - RequireMembers(SpecialMemberFunctionKind::CopyConstructor, - SpecialMemberFunctionKind::CopyAssignment); - } + if (!(DefinedSMFs & (Dtor | NonDefaultDtor | DefaultDtor))) + RequiredSMFs |= Dtor; - if (RequireFive && + if (getLangOpts().CPlusPlus11 && + ((DefinedSMFs & MoveOps) || !AllowMissingMoveFunctions) && + !(AllowImplicitlyDeletedCopyOrMove && + (ImplicitSMFs & DeletedSMFs & MoveOps) == MoveOps) && !(AllowMissingMoveFunctionsWhenCopyIsDeleted && - (IsDeleted(SpecialMemberFunctionKind::CopyConstructor) && - IsDeleted(SpecialMemberFunctionKind::CopyAssignment)))) { - assert(RequireThree); - RequireMembers(SpecialMemberFunctionKind::MoveConstructor, - SpecialMemberFunctionKind::MoveAssignment); - } + (DeletedSMFs & CopyOps) == CopyOps)) + RequiredSMFs |= MoveOps; - if (!MissingMembers.empty()) { - llvm::SmallVector<SpecialMemberFunctionKind, 5> DefinedMemberKinds; - for (const auto &Data : DefinedMembers) { - if (!Data.IsImplicit) - DefinedMemberKinds.push_back(Data.FunctionKind); - } - diag(ID.first, "class '%0' defines %1 but does not define %2") - << ID.second << cppcoreguidelines::join(DefinedMemberKinds, " and ") - << cppcoreguidelines::join(MissingMembers, " or "); - } + const SpecialMemberFunctions MissingSMFs = RequiredSMFs & ~DefinedSMFs; + if (!MissingSMFs) + return; + + diag(Class.getLocation(), "class %0 defines %1 but does not define %2") + << &Class << joinSMFs(DefinedSMFs, " and ") + << joinSMFs(MissingSMFs, " or "); } } // namespace clang::tidy::cppcoreguidelines diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.h b/clang-tools-extra/clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.h index 6d76e07078f3b..13dca7da45343 100644 --- a/clang-tools-extra/clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.h +++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.h @@ -11,8 +11,6 @@ #include "../ClangTidyCheck.h" -#include "llvm/ADT/DenseMapInfo.h" - namespace clang::tidy::cppcoreguidelines { /// Checks for classes where some, but not all, of the special member functions @@ -29,87 +27,18 @@ class SpecialMemberFunctionsCheck : public ClangTidyCheck { 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; - std::optional<TraversalKind> getCheckTraversalKind() const override; - - enum class SpecialMemberFunctionKind : uint8_t { - Destructor, - DefaultDestructor, - NonDefaultDestructor, - CopyConstructor, - CopyAssignment, - MoveConstructor, - MoveAssignment - }; - - struct SpecialMemberFunctionData { - SpecialMemberFunctionKind FunctionKind; - bool IsDeleted; - bool IsImplicit = false; - - bool operator==(const SpecialMemberFunctionData &Other) const { - return (Other.FunctionKind == FunctionKind) && - (Other.IsDeleted == IsDeleted); - } - }; - - using ClassDefId = std::pair<SourceLocation, std::string>; - - using ClassDefiningSpecialMembersMap = - llvm::DenseMap<ClassDefId, - llvm::SmallVector<SpecialMemberFunctionData, 5>>; + std::optional<TraversalKind> getCheckTraversalKind() const override { + return TK_IgnoreUnlessSpelledInSource; + } private: - void checkForMissingMembers( - const ClassDefId &ID, - llvm::ArrayRef<SpecialMemberFunctionData> DefinedMembers); - const bool AllowMissingMoveFunctions; const bool AllowSoleDefaultDtor; const bool AllowMissingMoveFunctionsWhenCopyIsDeleted; const bool AllowImplicitlyDeletedCopyOrMove; - ClassDefiningSpecialMembersMap ClassWithSpecialMembers; const bool IgnoreMacros; }; } // namespace clang::tidy::cppcoreguidelines -namespace llvm { -/// Specialization 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 ClassDefId getEmptyKey() { - return {DenseMapInfo<clang::SourceLocation>::getEmptyKey(), "EMPTY"}; - } - - static ClassDefId getTombstoneKey() { - return {DenseMapInfo<clang::SourceLocation>::getTombstoneKey(), - "TOMBSTONE"}; - } - - static unsigned getHashValue(const ClassDefId &Val) { - assert(Val != getEmptyKey() && "Cannot hash the empty key!"); - assert(Val != getTombstoneKey() && "Cannot hash the tombstone key!"); - - const std::hash<ClassDefId::second_type> SecondHash; - return Val.first.getHashValue() + SecondHash(Val.second); - } - - static bool isEqual(const ClassDefId &LHS, const 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_SPECIALMEMBERFUNCTIONSCHECK_H _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
