[clang-tidy] Add AllowImplicitlyDeletedCopyOrMove option to cppcoreguidelines-special-member-functions

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

@@ -25,25 +25,44 @@ SpecialMemberFunctionsCheck::SpecialMemberFunctionsCheck(
                                          "AllowMissingMoveFunctions", false)),
       AllowSoleDefaultDtor(Options.get("AllowSoleDefaultDtor", false)),
       AllowMissingMoveFunctionsWhenCopyIsDeleted(
-          Options.get("AllowMissingMoveFunctionsWhenCopyIsDeleted", false)) {}
+          Options.get("AllowMissingMoveFunctionsWhenCopyIsDeleted", false)),
+      AllowImplicitlyDeletedCopyOrMove(
+          Options.get("AllowImplicitlyDeletedCopyOrMove", false)) {}
 
 void SpecialMemberFunctionsCheck::storeOptions(
     ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "AllowMissingMoveFunctions", AllowMissingMoveFunctions);
   Options.store(Opts, "AllowSoleDefaultDtor", AllowSoleDefaultDtor);
   Options.store(Opts, "AllowMissingMoveFunctionsWhenCopyIsDeleted",
                 AllowMissingMoveFunctionsWhenCopyIsDeleted);
+  Options.store(Opts, "AllowImplicitlyDeletedCopyOrMove",
+                AllowImplicitlyDeletedCopyOrMove);
+}
+
+std::optional<TraversalKind>
+SpecialMemberFunctionsCheck::getCheckTraversalKind() const {
+  return AllowImplicitlyDeletedCopyOrMove ? TK_AsIs
+                                          : TK_IgnoreUnlessSpelledInSource;
 }
 
 void SpecialMemberFunctionsCheck::registerMatchers(MatchFinder *Finder) {
+  auto IsNotImplicitOrDeleted = anyOf(unless(isImplicit()), isDeleted());
+
   Finder->addMatcher(
       cxxRecordDecl(
-          eachOf(has(cxxDestructorDecl().bind("dtor")),
-                 has(cxxConstructorDecl(isCopyConstructor()).bind("copy-ctor")),
-                 has(cxxMethodDecl(isCopyAssignmentOperator())
+          unless(isImplicit()),
+          eachOf(has(cxxDestructorDecl(unless(isImplicit())).bind("dtor")),
+                 has(cxxConstructorDecl(isCopyConstructor(),
+                                        IsNotImplicitOrDeleted)
+                         .bind("copy-ctor")),
+                 has(cxxMethodDecl(isCopyAssignmentOperator(),
+                                   IsNotImplicitOrDeleted)
                          .bind("copy-assign")),
-                 has(cxxConstructorDecl(isMoveConstructor()).bind("move-ctor")),
-                 has(cxxMethodDecl(isMoveAssignmentOperator())
+                 has(cxxConstructorDecl(isMoveConstructor(),
+                                        IsNotImplicitOrDeleted)
+                         .bind("move-ctor")),
+                 has(cxxMethodDecl(isMoveAssignmentOperator(),
+                                   IsNotImplicitOrDeleted)
                          .bind("move-assign"))))
           .bind("class-def"),
       this);
@@ -127,7 +146,8 @@ void SpecialMemberFunctionsCheck::check(
   for (const auto &KV : Matchers)
     if (const auto *MethodDecl =
             Result.Nodes.getNodeAs<CXXMethodDecl>(KV.first)) {
-      StoreMember({KV.second, MethodDecl->isDeleted()});
+      StoreMember(
+          {KV.second, MethodDecl->isDeleted(), MethodDecl->isImplicit()});
     }
 }
 
@@ -144,7 +164,13 @@ void SpecialMemberFunctionsCheck::checkForMissingMembers(
 
   auto HasMember = [&](SpecialMemberFunctionKind Kind) {
     return llvm::any_of(DefinedMembers, [Kind](const auto &Data) {
-      return Data.FunctionKind == Kind;
+      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;
     });
   };
 
@@ -154,9 +180,17 @@ void SpecialMemberFunctionsCheck::checkForMissingMembers(
     });
   };
 
-  auto RequireMember = [&](SpecialMemberFunctionKind Kind) {
-    if (!HasMember(Kind))
-      MissingMembers.push_back(Kind);
+  auto RequireMembers = [&](SpecialMemberFunctionKind Kind1,
+                            SpecialMemberFunctionKind Kind2) {
+    if (AllowImplicitlyDeletedCopyOrMove && HasImplicitDeletedMember(Kind1) &&
+        HasImplicitDeletedMember(Kind2))
+      return;
+
+    if (!HasMember(Kind1))
+      MissingMembers.push_back(Kind1);
+
+    if (!HasMember(Kind2))
+      MissingMembers.push_back(Kind2);
   };
 
   bool RequireThree =
@@ -180,23 +214,25 @@ void SpecialMemberFunctionsCheck::checkForMissingMembers(
         !HasMember(SpecialMemberFunctionKind::NonDefaultDestructor))
       MissingMembers.push_back(SpecialMemberFunctionKind::Destructor);
 
-    RequireMember(SpecialMemberFunctionKind::CopyConstructor);
-    RequireMember(SpecialMemberFunctionKind::CopyAssignment);
+    RequireMembers(SpecialMemberFunctionKind::CopyConstructor,
+                   SpecialMemberFunctionKind::CopyAssignment);
   }
 
   if (RequireFive &&
       !(AllowMissingMoveFunctionsWhenCopyIsDeleted &&
         (IsDeleted(SpecialMemberFunctionKind::CopyConstructor) &&
          IsDeleted(SpecialMemberFunctionKind::CopyAssignment)))) {
     assert(RequireThree);
-    RequireMember(SpecialMemberFunctionKind::MoveConstructor);
-    RequireMember(SpecialMemberFunctionKind::MoveAssignment);
+    RequireMembers(SpecialMemberFunctionKind::MoveConstructor,
+                   SpecialMemberFunctionKind::MoveAssignment);
   }
 
   if (!MissingMembers.empty()) {
     llvm::SmallVector<SpecialMemberFunctionKind, 5> DefinedMemberKinds;
-    llvm::transform(DefinedMembers, std::back_inserter(DefinedMemberKinds),
-                    [](const auto &Data) { return Data.FunctionKind; });
+    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 ");

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

@@ -30,9 +30,8 @@ class SpecialMemberFunctionsCheck : public ClangTidyCheck {
   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 {
-    return TK_IgnoreUnlessSpelledInSource;
-  }
+  std::optional<TraversalKind> getCheckTraversalKind() const override;
+
   enum class SpecialMemberFunctionKind : uint8_t {
     Destructor,
     DefaultDestructor,
@@ -46,6 +45,7 @@ class SpecialMemberFunctionsCheck : public ClangTidyCheck {
   struct SpecialMemberFunctionData {
     SpecialMemberFunctionKind FunctionKind;
     bool IsDeleted;
+    bool IsImplicit = false;
 
     bool operator==(const SpecialMemberFunctionData &Other) const {
       return (Other.FunctionKind == FunctionKind) &&
@@ -67,6 +67,7 @@ class SpecialMemberFunctionsCheck : public ClangTidyCheck {
   const bool AllowMissingMoveFunctions;
   const bool AllowSoleDefaultDtor;
   const bool AllowMissingMoveFunctionsWhenCopyIsDeleted;
+  const bool AllowImplicitlyDeletedCopyOrMove;
   ClassDefiningSpecialMembersMap ClassWithSpecialMembers;
 };
 

clang-tools-extra/docs/ReleaseNotes.rst

@@ -246,6 +246,12 @@ Changes in existing checks
   <clang-tidy/checks/cppcoreguidelines/use-default-member-init>`. Fixed
   incorrect hints when using list-initialization.
 
+- Improved :doc:`cppcoreguidelines-special-member-functions
+  <clang-tidy/checks/cppcoreguidelines/special-member-functions>` check with a
+  new option `AllowImplicitlyDeletedCopyOrMove`, which removes the requirement
+  for explicit copy or move special member functions when they are already
+  implicitly deleted.
+
 - Improved :doc:`google-build-namespaces
   <clang-tidy/checks/google/build-namespaces>` check by replacing the local
   option `HeaderFileExtensions` by the global option of the same name.

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

@@ -45,9 +45,10 @@ Options
 
 .. option:: AllowMissingMoveFunctions
 
-   When set to `true` (default is `false`), 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. With this option enabled, the following class won't be flagged:
+   When set to `true` (default is `false`), 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. With this
+   option enabled, the following class won't be flagged:
 
    .. code-block:: c++
 
@@ -59,10 +60,11 @@ Options
 
 .. option:: AllowMissingMoveFunctionsWhenCopyIsDeleted
 
-   When set to `true` (default is `false`), this check doesn't flag classes which define deleted copy
-   operations but don't define move operations. This flag 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:
+   When set to `true` (default is `false`), this check doesn't flag classes
+   which define deleted copy operations but don't define move operations. This
+   flag is related to Google C++ Style Guide `Copyable and Movable Types
+   <https://google.github.io/styleguide/cppguide.html#Copyable_Movable_Types>`_.
+   With this option enabled, the following class won't be flagged:
 
    .. code-block:: c++
 
@@ -71,3 +73,15 @@ Options
        A& operator=(const A&) = delete;
        ~A();
      };
+
+.. option:: AllowImplicitlyDeletedCopyOrMove
+
+   When set to `true` (default is `false`), this check doesn't flag classes
+   which implicitly delete copy or move operations.
+   With this option enabled, the following class won't be flagged:
+
+   .. code-block:: c++
+
+      struct A : boost::noncopyable {
+        ~A() { std::cout << "dtor\n"; }
+      };

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/special-member-functions-relaxed.cpp

@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy %s cppcoreguidelines-special-member-functions %t -- -config="{CheckOptions: {cppcoreguidelines-special-member-functions.AllowMissingMoveFunctions: true, cppcoreguidelines-special-member-functions.AllowSoleDefaultDtor: true}}" --
+// RUN: %check_clang_tidy %s cppcoreguidelines-special-member-functions %t -- -config="{CheckOptions: {cppcoreguidelines-special-member-functions.AllowMissingMoveFunctions: true, cppcoreguidelines-special-member-functions.AllowSoleDefaultDtor: true, cppcoreguidelines-special-member-functions.AllowImplicitlyDeletedCopyOrMove: true}}" --
 
 // Don't warn on destructors without definitions, they might be defaulted in another TU.
 class DeclaresDestructor {
@@ -34,12 +34,13 @@ class DefinesCopyAssignment {
 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]
+// CHECK-MESSAGES: [[@LINE-3]]:7: warning: class 'DefinesMoveConstructor' defines a move constructor but does not define a destructor 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]
+// CHECK-MESSAGES: [[@LINE-3]]:7: warning: class 'DefinesMoveAssignment' defines a move assignment operator but does not define a destructor or a move constructor [cppcoreguidelines-special-member-functions]
+
 class DefinesNothing {
 };
 
@@ -81,3 +82,22 @@ struct TemplateClass {
 // This should not cause problems.
 TemplateClass<int> InstantiationWithInt;
 TemplateClass<double> InstantiationWithDouble;
+
+struct NoCopy
+{
+  NoCopy() = default;
+  ~NoCopy() = default;
+
+  NoCopy(const NoCopy&) = delete;
+  NoCopy(NoCopy&&) = delete;
+
+  NoCopy& operator=(const NoCopy&) = delete;
+  NoCopy& operator=(NoCopy&&) = delete;
+};
+
+// CHECK-MESSAGES: [[@LINE+1]]:8: warning: class 'NonCopyable' defines a copy constructor but does not define a destructor or a copy assignment operator [cppcoreguidelines-special-member-functions]
+struct NonCopyable : NoCopy
+{
+  NonCopyable() = default;
+  NonCopyable(const NonCopyable&) = delete;
+};