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; _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
