[clang-tools-extra] aa56e66 - [clang-tidy] Tweak 'rule of 3/5' checks to allow defaulting a destructor outside the class.

[Roy Jacobson via cfe-commits]

Author: Roy Jacobson
Date: 2023-02-26T15:55:54+02:00
New Revision: aa56e66bf7520512fd1209cbb7d099604a5f6277

URL: https://github.com/llvm/llvm-project/commit/aa56e66bf7520512fd1209cbb7d099604a5f6277
DIFF: https://github.com/llvm/llvm-project/commit/aa56e66bf7520512fd1209cbb7d099604a5f6277.diff

LOG: [clang-tidy] Tweak 'rule of 3/5' checks to allow defaulting a destructor outside the class.

A somewhat common code-pattern is to default a destructor in the source file and not in the header.
For example, this is the way to use smart pointers with forward-declared classes:

```c++

struct Impl;
struct A {
  ~A(); // Can't be defaulted in the header.

private:
  std::unique_ptr<Impl> impl;
};
```

To be able to use this check with this pattern, I modified the behavior with `AllowSoleDefaultDtor`
to not trigger on destructors if they aren't defined yet.
Since a declared destructor should still be defined somewhere in the program, this
won't miss bad classes, just diagnose on less translation units.

Reviewed By: carlosgalvezp

Differential Revision: https://reviews.llvm.org/D143851

Added: 
    

Modified: 
    clang-tools-extra/clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.cpp
    clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/special-member-functions.rst
    clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/special-member-functions-cxx-03.cpp
    clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/special-member-functions-relaxed.cpp
    clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/special-member-functions.cpp

Removed: 
    


################################################################################
diff  --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.cpp b/clang-tools-extra/clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.cpp
index 9bea2a78fa953..d2117c67a76d0 100644
--- a/clang-tools-extra/clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.cpp
+++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.cpp
@@ -108,10 +108,14 @@ void SpecialMemberFunctionsCheck::check(
   };
 
   if (const auto *Dtor = Result.Nodes.getNodeAs<CXXMethodDecl>("dtor")) {
-    StoreMember({Dtor->isDefaulted()
-                     ? SpecialMemberFunctionKind::DefaultDestructor
-                     : SpecialMemberFunctionKind::NonDefaultDestructor,
-                 Dtor->isDeleted()});
+    SpecialMemberFunctionKind DestructorType =
+        SpecialMemberFunctionKind::Destructor;
+    if (Dtor->isDefined()) {
+      DestructorType = Dtor->getDefinition()->isDefaulted()
+                           ? SpecialMemberFunctionKind::DefaultDestructor
+                           : SpecialMemberFunctionKind::NonDefaultDestructor;
+    }
+    StoreMember({DestructorType, Dtor->isDeleted()});
   }
 
   std::initializer_list<std::pair<std::string, SpecialMemberFunctionKind>>
@@ -158,7 +162,8 @@ void SpecialMemberFunctionsCheck::checkForMissingMembers(
   bool RequireThree =
       HasMember(SpecialMemberFunctionKind::NonDefaultDestructor) ||
       (!AllowSoleDefaultDtor &&
-       HasMember(SpecialMemberFunctionKind::DefaultDestructor)) ||
+       (HasMember(SpecialMemberFunctionKind::Destructor) ||
+        HasMember(SpecialMemberFunctionKind::DefaultDestructor))) ||
       HasMember(SpecialMemberFunctionKind::CopyConstructor) ||
       HasMember(SpecialMemberFunctionKind::CopyAssignment) ||
       HasMember(SpecialMemberFunctionKind::MoveConstructor) ||
@@ -170,7 +175,8 @@ void SpecialMemberFunctionsCheck::checkForMissingMembers(
                      HasMember(SpecialMemberFunctionKind::MoveAssignment);
 
   if (RequireThree) {
-    if (!HasMember(SpecialMemberFunctionKind::DefaultDestructor) &&
+    if (!HasMember(SpecialMemberFunctionKind::Destructor) &&
+        !HasMember(SpecialMemberFunctionKind::DefaultDestructor) &&
         !HasMember(SpecialMemberFunctionKind::NonDefaultDestructor))
       MissingMembers.push_back(SpecialMemberFunctionKind::Destructor);
 

diff  --git a/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/special-member-functions.rst b/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/special-member-functions.rst
index 92815e216242c..2d8b842be5942 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/special-member-functions.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/special-member-functions.rst
@@ -25,15 +25,25 @@ Options
 
 .. option:: AllowSoleDefaultDtor
 
-   When set to `true` (default is `false`), this check doesn't flag classes with a sole, explicitly
-   defaulted destructor. An example for such a class is:
+   When set to `true` (default is `false`), this check will only trigger on
+   destructors if they are defined and not defaulted.
 
    .. code-block:: c++
 
-     struct A {
+     struct A { // This is fine.
        virtual ~A() = default;
      };
 
+     struct B { // This is not fine.
+       ~B() {}
+     };
+
+     struct C {
+       // This is not checked, because the destructor might be defaulted in
+       // another translation unit.
+       ~C();
+     };
+
 .. option:: AllowMissingMoveFunctions
 
    When set to `true` (default is `false`), this check doesn't flag classes which define no move

diff  --git a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/special-member-functions-cxx-03.cpp b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/special-member-functions-cxx-03.cpp
index 10f3955846d71..272adff32677a 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/special-member-functions-cxx-03.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/special-member-functions-cxx-03.cpp
@@ -3,7 +3,7 @@
 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]
+// 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 &);

diff  --git a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/special-member-functions-relaxed.cpp b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/special-member-functions-relaxed.cpp
index 72248d3e05feb..4bdaff5a7cb26 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/special-member-functions-relaxed.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/special-member-functions-relaxed.cpp
@@ -1,14 +1,26 @@
 // RUN: %check_clang_tidy %s cppcoreguidelines-special-member-functions %t -- -config="{CheckOptions: [{key: cppcoreguidelines-special-member-functions.AllowMissingMoveFunctions, value: true}, {key: cppcoreguidelines-special-member-functions.AllowSoleDefaultDtor, value: true}]}" --
 
+// Don't warn on destructors without definitions, they might be defaulted in another TU.
+class DeclaresDestructor {
+  ~DeclaresDestructor();
+};
+
 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]
+DefinesDestructor::~DefinesDestructor() {}
+// CHECK-MESSAGES: [[@LINE-4]]: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 DefinesDefaultedDestructorOutside {
+  ~DefinesDefaultedDestructorOutside();
+};
+
+DefinesDefaultedDestructorOutside::~DefinesDefaultedDestructorOutside() = default;
+
 class DefinesCopyConstructor {
   DefinesCopyConstructor(const DefinesCopyConstructor &);
 };

diff  --git a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/special-member-functions.cpp b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/special-member-functions.cpp
index 013ba8623a62b..60c945c8e20c3 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/special-member-functions.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/special-member-functions.cpp
@@ -3,7 +3,7 @@
 class DefinesDestructor {
   ~DefinesDestructor();
 };
-// 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]
+// 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 DefinesDefaultedDestructor {
   ~DefinesDefaultedDestructor() = default;