[clang-tools-extra] [clang-tidy] Add AllowImplicitlyDeletedCopyOrMove option to cppcoreguidelines-special-member-functions (PR #71683)
Piotr Zegar via cfe-commits
cfe-commits at lists.llvm.org
Wed Nov 8 06:30:56 PST 2023
https://github.com/PiotrZSL created https://github.com/llvm/llvm-project/pull/71683
Improved 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.
Closes #62392
>From 5c6db0ec4850bed27f94839d0f018d66fa1c57f4 Mon Sep 17 00:00:00 2001
From: Piotr Zegar <me at piotrzegar.pl>
Date: Wed, 8 Nov 2023 13:31:28 +0000
Subject: [PATCH] [clang-tidy] Add AllowImplicitlyDeletedCopyOrMove option to
cppcoreguidelines-special-member-functions
Improved 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.
---
.../SpecialMemberFunctionsCheck.cpp | 70 ++++++++++++++-----
.../SpecialMemberFunctionsCheck.h | 7 +-
clang-tools-extra/docs/ReleaseNotes.rst | 6 ++
.../special-member-functions.rst | 28 ++++++--
.../special-member-functions-relaxed.cpp | 26 ++++++-
5 files changed, 107 insertions(+), 30 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.cpp b/clang-tools-extra/clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.cpp
index d2117c67a76d0b6..ed76ac665049d1c 100644
--- a/clang-tools-extra/clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.cpp
+++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.cpp
@@ -25,7 +25,9 @@ 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) {
@@ -33,17 +35,34 @@ void SpecialMemberFunctionsCheck::storeOptions(
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,8 +214,8 @@ void SpecialMemberFunctionsCheck::checkForMissingMembers(
!HasMember(SpecialMemberFunctionKind::NonDefaultDestructor))
MissingMembers.push_back(SpecialMemberFunctionKind::Destructor);
- RequireMember(SpecialMemberFunctionKind::CopyConstructor);
- RequireMember(SpecialMemberFunctionKind::CopyAssignment);
+ RequireMembers(SpecialMemberFunctionKind::CopyConstructor,
+ SpecialMemberFunctionKind::CopyAssignment);
}
if (RequireFive &&
@@ -189,14 +223,16 @@ void SpecialMemberFunctionsCheck::checkForMissingMembers(
(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 ");
diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.h b/clang-tools-extra/clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.h
index 6042f0fd6cb0543..9ebc03ed2fa139d 100644
--- a/clang-tools-extra/clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.h
+++ b/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;
};
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index fe8c7175d554c7b..934acdca2b4f157 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -290,6 +290,12 @@ Changes in existing checks
to ignore unused parameters when they are marked as unused and parameters of
deleted functions and constructors.
+- 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:`llvm-namespace-comment
<clang-tidy/checks/llvm/namespace-comment>` check to provide fixes for
``inline`` namespaces in the same format as :program:`clang-format`.
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 176956d6cb2bd72..20f898fdab93053 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
@@ -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"; }
+ };
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 0c17f57968a990b..26142ccc835f294 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,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;
+};
More information about the cfe-commits
mailing list