royjacobson created this revision.
royjacobson added reviewers: carlosgalvezp, njames93, aaron.ballman.
Herald added subscribers: kbarton, xazax.hun, nemanjai.
Herald added a project: All.
royjacobson requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.
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 doesn't
won't miss bad classes, just diagnose on less translation units.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D143851
Files:
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
Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/special-member-functions.cpp
===================================================================
--- clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/special-member-functions.cpp
+++ 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;
Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/special-member-functions-relaxed.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-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 &);
};
Index: 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-cxx-03.cpp
+++ 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 &);
Index: clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/special-member-functions.rst
===================================================================
--- clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/special-member-functions.rst
+++ clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/special-member-functions.rst
@@ -25,15 +25,22 @@
.. 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 destuctor 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
Index: clang-tools-extra/clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.cpp
+++ clang-tools-extra/clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.cpp
@@ -108,10 +108,14 @@
};
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 @@
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 @@
HasMember(SpecialMemberFunctionKind::MoveAssignment);
if (RequireThree) {
- if (!HasMember(SpecialMemberFunctionKind::DefaultDestructor) &&
+ if (!HasMember(SpecialMemberFunctionKind::Destructor) &&
+ !HasMember(SpecialMemberFunctionKind::DefaultDestructor) &&
!HasMember(SpecialMemberFunctionKind::NonDefaultDestructor))
MissingMembers.push_back(SpecialMemberFunctionKind::Destructor);
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
