[clang-tidy] Added AllowMissingMoveFunctionsWhenCopyIsDeleted flag to cppcoreguidelines-special-member-functions

[pbaran]

Summary

The flag allows classes to don't define move operations when copy operations are explicitly deleted. This flag is related to Google C++ Style Guide https://google.github.io/styleguide/cppguide.html#Copyable_Movable_Types

[pbaran]

Removed unintendedly added comment

[aaron.ballman]

LGTM!

[pbaran]

Thank you! Could you please integrate it for me? I do not have access rights

[aaron.ballman]

In D75745#1923913, @pbaran wrote:

Thank you! Could you please integrate it for me? I do not have access rights

Happy to do so; I've commit in 2f20417ef04781cd5019b9a99b85df4790bdf525, thank you for the patch!

clang-tools-extra/clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.h

enum class SpecialMemberFunctionKind : uint8_t {
     DefaultDestructor,
     NonDefaultDestructor,
     CopyConstructor,
     CopyAssignment,
     MoveConstructor,
     MoveAssignment
   };
 
+  struct SpecialMemberFunctionData {
+    SpecialMemberFunctionKind FunctionKind;
+    bool IsDeleted;
+
+    bool operator==(const SpecialMemberFunctionData &Other) {
+      return (Other.FunctionKind == FunctionKind) &&
+             (Other.IsDeleted == IsDeleted);
+    }
+  };
+
   using ClassDefId = std::pair<SourceLocation, std::string>;
 
   using ClassDefiningSpecialMembersMap =
       llvm::DenseMap<ClassDefId,
-                     llvm::SmallVector<SpecialMemberFunctionKind, 5>>;
+                     llvm::SmallVector<SpecialMemberFunctionData, 5>>;
 
 private:
   void checkForMissingMembers(
       const ClassDefId &ID,
-      llvm::ArrayRef<SpecialMemberFunctionKind> DefinedSpecialMembers);
+      llvm::ArrayRef<SpecialMemberFunctionData> DefinedSpecialMembers);
 
   const bool AllowMissingMoveFunctions;
   const bool AllowSoleDefaultDtor;
+  const bool AllowMissingMoveFunctionsWhenCopyIsDeleted;
   ClassDefiningSpecialMembersMap ClassWithSpecialMembers;
 };
 
 } // namespace cppcoreguidelines
 } // namespace tidy
 } // namespace clang
 
 namespace llvm {

clang-tools-extra/clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.cpp

 namespace clang {
 namespace tidy {
 namespace cppcoreguidelines {
 
 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) {
   Finder->addMatcher(
       cxxRecordDecl(
           eachOf(
               has(cxxDestructorDecl(unless(isImplicit())).bind("dtor")),
               has(cxxConstructorDecl(isCopyConstructor(), unless(isImplicit()))

 void SpecialMemberFunctionsCheck::check(
     const MatchFinder::MatchResult &Result) {
   const auto *MatchedDecl = Result.Nodes.getNodeAs<CXXRecordDecl>("class-def");
   if (!MatchedDecl)
     return;
 
   ClassDefId ID(MatchedDecl->getLocation(), std::string(MatchedDecl->getName()));
 
-  auto StoreMember = [this, &ID](SpecialMemberFunctionKind Kind) {
-    llvm::SmallVectorImpl<SpecialMemberFunctionKind> &Members =
+  auto StoreMember = [this, &ID](SpecialMemberFunctionData data) {
+    llvm::SmallVectorImpl<SpecialMemberFunctionData> &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<CXXMethodDecl>("dtor")) {
-    StoreMember(Dtor->isDefaulted()
+    StoreMember({Dtor->isDefaulted()
                      ? SpecialMemberFunctionKind::DefaultDestructor
-                    : SpecialMemberFunctionKind::NonDefaultDestructor);
+                     : SpecialMemberFunctionKind::NonDefaultDestructor,
+                 Dtor->isDeleted()});
   }
 
   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 (Result.Nodes.getNodeAs<CXXMethodDecl>(KV.first)) {
-      StoreMember(KV.second);
+    if (const auto *MethodDecl =
+            Result.Nodes.getNodeAs<CXXMethodDecl>(KV.first)) {
+      StoreMember({KV.second, MethodDecl->isDeleted()});
     }
 }
 
 void SpecialMemberFunctionsCheck::onEndOfTranslationUnit() {
   for (const auto &C : ClassWithSpecialMembers) {
     checkForMissingMembers(C.first, C.second);
   }
 }
 
 void SpecialMemberFunctionsCheck::checkForMissingMembers(
     const ClassDefId &ID,
-    llvm::ArrayRef<SpecialMemberFunctionKind> DefinedMembers) {
+    llvm::ArrayRef<SpecialMemberFunctionData> DefinedMembers) {
   llvm::SmallVector<SpecialMemberFunctionKind, 5> 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) {
     if (!HasMember(Kind))
       MissingMembers.push_back(Kind);
   };
 
   bool RequireThree =
if (RequireThree) {
     if (!HasMember(SpecialMemberFunctionKind::DefaultDestructor) &&
         !HasMember(SpecialMemberFunctionKind::NonDefaultDestructor))
       MissingMembers.push_back(SpecialMemberFunctionKind::Destructor);
 
     RequireMember(SpecialMemberFunctionKind::CopyConstructor);
     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<SpecialMemberFunctionKind, 5> 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
 } // namespace tidy
 } // namespace clang

clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-special-member-functions.rst

.. option:: AllowMissingMoveFunctions
    move constructor or move assignment operator. With this option enabled, the following class won't be flagged:
    
    .. code-block:: c++
    
      struct A {
        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();
+     };

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-special-member-functions-allow-missing-move-when-copy-is-deleted.cpp

+// 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;
+};