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

[royjacobson]

Summary

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.

[ccotter]

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

41

typo: destuctor->destructor

[Eugene.Zelenko]

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

28–29

Please follos 80 characters limit.

41

Ditto.

[royjacobson]

Address CR comments.

[PiotrZSL]

Thing is that same issue may happen with all other members, what about copy/move constructors defaulted in .cpp (just to speed up compilation for classes with many members).
Best thing would simply to check if all definitions are available. In that case issues would be reported only for source file that corresponds to header file with defined class.
This would also reduce amount of generated warnings for a big project that include such header with class in many places.

Other good option would be to excluded classes defined in system headers, no point to check if some boost class or std::vector got proper constructors.

[royjacobson]

In D143851#4126453, @ClockMan wrote:

Thing is that same issue may happen with all other members, what about copy/move constructors defaulted in .cpp (just to speed up compilation for classes with many members).
Best thing would simply to check if all definitions are available. In that case issues would be reported only for source file that corresponds to header file with defined class.
This would also reduce amount of generated warnings for a big project that include such header with class in many places.

Other good option would be to excluded classes defined in system headers, no point to check if some boost class or std::vector got proper constructors.

Defaulting destructors is usually fine, but explicitly defaulting copy/move constructors implicitly deletes the other special member functions which this check should diagnose.
This is why this option was only available for destructors in the first place, I imagine.

About the second point - this sounds pretty general, doesn't clang-tidy already filter diagnostics in system headers? At least Clang does.

[carlosgalvezp]

This is why this option was only available for destructors in the first place, I imagine.

The cppcoreguidelines are a bit tricky to work with. Some of the rules are too strict to reasonably apply them in practice. We have brought this up with the authors of the guidelines but they have rejected proposals for modification. That's why in clang-tidy we aim at having by default the strict behavior so the check follows the rules "as-is", while also adding options that allow users to slightly deviate from the rules, to be more pragmatic. Thus if we want some new behavior that deviates from the rule text as it's written we should preferably implement it as an option.

About the second point - this sounds pretty general, doesn't clang-tidy already filter diagnostics in system headers? At least Clang does.

Yes, clang-tidy will not issue warnings in system headers, so checks should not need to explicitly handle that.

[PiotrZSL]

In D143851#4126615, @carlosgalvezp wrote:

Yes, clang-tidy will not issue warnings in system headers, so checks should not need to explicitly handle that.

That's not so simple. At work we use clang-tidy for few big projects. I had to explicitly add to some checks unless(isExpansionInSystemHeader()) to gain some performance improvement of clang-tidy. This check is one of them.
Problem is that some matchers or check methods in checks are heavy, and when you got lot of system headers (boost, STL, generated interfaces, message structs, ...) you may end up easily with hundreds of hidden issues from single translation unit.
For some checks explicit exclude of system headers gives ~50% performance improvement, that result in faster CI feedback.

Best would be to have something more generic like: if --system-headers is not used, then exclude code as early as possible, before even constructing diagnostic information.

[carlosgalvezp]

That's not so simple. At work we use clang-tidy for few big projects. I had to explicitly add to some checks unless(isExpansionInSystemHeader()) to gain some performance improvement of clang-tidy. This check is one of them.
Problem is that some matchers or check methods in checks are heavy, and when you got lot of system headers (boost, STL, generated interfaces, message structs, ...) you may end up easily with hundreds of hidden issues from single translation unit.
For some checks explicit exclude of system headers gives ~50% performance improvement, that result in faster CI feedback.

Best would be to have something more generic like: if --system-headers is not used, then exclude code as early as possible, before even constructing diagnostic information.

Yes, I meant from a "diagnostic" point of view. I agree that performance-wise it's a lot of work that is discarded at the end and would be good to avoid as early as possible. This sounds like something all checks would benefit from, so finding a general solution to that problem would be good. I believe the best forum to discuss this would be an RFC in Discourse, make sure to tag @njames93 as Code Owner.

[royjacobson]

friendly ping

[carlosgalvezp]

LGTM, thanks for the contribution! Do you have commit rights or would you like that we land it for you? If so, please provide name and email for attribution.

[royjacobson]

In D143851#4153270, @carlosgalvezp wrote:

LGTM, thanks for the contribution! Do you have commit rights or would you like that we land it for you? If so, please provide name and email for attribution.

I have commit rights, but this is my first clang-tidy contribution :) Thanks!

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

void SpecialMemberFunctionsCheck::check(
   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")) {
-    StoreMember({Dtor->isDefaulted()
+    SpecialMemberFunctionKind DestructorType =
+        SpecialMemberFunctionKind::Destructor;
+    if (Dtor->isDefined()) {
+      DestructorType = Dtor->getDefinition()->isDefaulted()
                            ? SpecialMemberFunctionKind::DefaultDestructor
-                     : SpecialMemberFunctionKind::NonDefaultDestructor,
-                 Dtor->isDeleted()});
+                           : SpecialMemberFunctionKind::NonDefaultDestructor;
+    }
+    StoreMember({DestructorType, 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}};
 
void SpecialMemberFunctionsCheck::checkForMissingMembers(
   auto RequireMember = [&](SpecialMemberFunctionKind Kind) {
     if (!HasMember(Kind))
       MissingMembers.push_back(Kind);
   };
 
   bool RequireThree =
       HasMember(SpecialMemberFunctionKind::NonDefaultDestructor) ||
       (!AllowSoleDefaultDtor &&
-       HasMember(SpecialMemberFunctionKind::DefaultDestructor)) ||
+       (HasMember(SpecialMemberFunctionKind::Destructor) ||
+        HasMember(SpecialMemberFunctionKind::DefaultDestructor))) ||
       HasMember(SpecialMemberFunctionKind::CopyConstructor) ||
       HasMember(SpecialMemberFunctionKind::CopyAssignment) ||
       HasMember(SpecialMemberFunctionKind::MoveConstructor) ||
       HasMember(SpecialMemberFunctionKind::MoveAssignment);
 
   bool RequireFive = (!AllowMissingMoveFunctions && RequireThree &&
                       getLangOpts().CPlusPlus11) ||
                      HasMember(SpecialMemberFunctionKind::MoveConstructor) ||
                      HasMember(SpecialMemberFunctionKind::MoveAssignment);
 
   if (RequireThree) {
-    if (!HasMember(SpecialMemberFunctionKind::DefaultDestructor) &&
+    if (!HasMember(SpecialMemberFunctionKind::Destructor) &&
+        !HasMember(SpecialMemberFunctionKind::DefaultDestructor) &&
         !HasMember(SpecialMemberFunctionKind::NonDefaultDestructor))
       MissingMembers.push_back(SpecialMemberFunctionKind::Destructor);
 
     RequireMember(SpecialMemberFunctionKind::CopyConstructor);
     RequireMember(SpecialMemberFunctionKind::CopyAssignment);
   }
 
   if (RequireFive &&

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

 
 https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#c21-if-you-define-or-delete-any-default-operation-define-or-delete-them-all.
 
 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
    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++
 

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

 // RUN: %check_clang_tidy -std=c++98 %s cppcoreguidelines-special-member-functions %t
 
 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 &);
 };
 // 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 &);

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

 // 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 &);
 };
 // 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 &);
 };

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

 // RUN: %check_clang_tidy %s cppcoreguidelines-special-member-functions %t
 
 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;
 };
 // CHECK-MESSAGES: [[@LINE-3]]:7: warning: class 'DefinesDefaultedDestructor' 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 DefinesCopyConstructor {
   DefinesCopyConstructor(const DefinesCopyConstructor &);