Skip to content

Commit 54c6ee9

Browse files
PiotrZSLPiotrZSL
authored
[clang-tidy] Add AllowImplicitlyDeletedCopyOrMove option to cppcoreguidelines-special-member-functions (#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
1 parent ba34476 commit 54c6ee9

File tree

5 files changed

+107
-30
lines changed

5 files changed

+107
-30
lines changed

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

Lines changed: 53 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -25,25 +25,44 @@ SpecialMemberFunctionsCheck::SpecialMemberFunctionsCheck(
2525
"AllowMissingMoveFunctions", false)),
2626
AllowSoleDefaultDtor(Options.get("AllowSoleDefaultDtor", false)),
2727
AllowMissingMoveFunctionsWhenCopyIsDeleted(
28-
Options.get("AllowMissingMoveFunctionsWhenCopyIsDeleted", false)) {}
28+
Options.get("AllowMissingMoveFunctionsWhenCopyIsDeleted", false)),
29+
AllowImplicitlyDeletedCopyOrMove(
30+
Options.get("AllowImplicitlyDeletedCopyOrMove", false)) {}
2931

3032
void SpecialMemberFunctionsCheck::storeOptions(
3133
ClangTidyOptions::OptionMap &Opts) {
3234
Options.store(Opts, "AllowMissingMoveFunctions", AllowMissingMoveFunctions);
3335
Options.store(Opts, "AllowSoleDefaultDtor", AllowSoleDefaultDtor);
3436
Options.store(Opts, "AllowMissingMoveFunctionsWhenCopyIsDeleted",
3537
AllowMissingMoveFunctionsWhenCopyIsDeleted);
38+
Options.store(Opts, "AllowImplicitlyDeletedCopyOrMove",
39+
AllowImplicitlyDeletedCopyOrMove);
40+
}
41+
42+
std::optional<TraversalKind>
43+
SpecialMemberFunctionsCheck::getCheckTraversalKind() const {
44+
return AllowImplicitlyDeletedCopyOrMove ? TK_AsIs
45+
: TK_IgnoreUnlessSpelledInSource;
3646
}
3747

3848
void SpecialMemberFunctionsCheck::registerMatchers(MatchFinder *Finder) {
49+
auto IsNotImplicitOrDeleted = anyOf(unless(isImplicit()), isDeleted());
50+
3951
Finder->addMatcher(
4052
cxxRecordDecl(
41-
eachOf(has(cxxDestructorDecl().bind("dtor")),
42-
has(cxxConstructorDecl(isCopyConstructor()).bind("copy-ctor")),
43-
has(cxxMethodDecl(isCopyAssignmentOperator())
53+
unless(isImplicit()),
54+
eachOf(has(cxxDestructorDecl(unless(isImplicit())).bind("dtor")),
55+
has(cxxConstructorDecl(isCopyConstructor(),
56+
IsNotImplicitOrDeleted)
57+
.bind("copy-ctor")),
58+
has(cxxMethodDecl(isCopyAssignmentOperator(),
59+
IsNotImplicitOrDeleted)
4460
.bind("copy-assign")),
45-
has(cxxConstructorDecl(isMoveConstructor()).bind("move-ctor")),
46-
has(cxxMethodDecl(isMoveAssignmentOperator())
61+
has(cxxConstructorDecl(isMoveConstructor(),
62+
IsNotImplicitOrDeleted)
63+
.bind("move-ctor")),
64+
has(cxxMethodDecl(isMoveAssignmentOperator(),
65+
IsNotImplicitOrDeleted)
4766
.bind("move-assign"))))
4867
.bind("class-def"),
4968
this);
@@ -127,7 +146,8 @@ void SpecialMemberFunctionsCheck::check(
127146
for (const auto &KV : Matchers)
128147
if (const auto *MethodDecl =
129148
Result.Nodes.getNodeAs<CXXMethodDecl>(KV.first)) {
130-
StoreMember({KV.second, MethodDecl->isDeleted()});
149+
StoreMember(
150+
{KV.second, MethodDecl->isDeleted(), MethodDecl->isImplicit()});
131151
}
132152
}
133153

@@ -144,7 +164,13 @@ void SpecialMemberFunctionsCheck::checkForMissingMembers(
144164

145165
auto HasMember = [&](SpecialMemberFunctionKind Kind) {
146166
return llvm::any_of(DefinedMembers, [Kind](const auto &Data) {
147-
return Data.FunctionKind == Kind;
167+
return Data.FunctionKind == Kind && !Data.IsImplicit;
168+
});
169+
};
170+
171+
auto HasImplicitDeletedMember = [&](SpecialMemberFunctionKind Kind) {
172+
return llvm::any_of(DefinedMembers, [Kind](const auto &Data) {
173+
return Data.FunctionKind == Kind && Data.IsImplicit && Data.IsDeleted;
148174
});
149175
};
150176

@@ -154,9 +180,17 @@ void SpecialMemberFunctionsCheck::checkForMissingMembers(
154180
});
155181
};
156182

157-
auto RequireMember = [&](SpecialMemberFunctionKind Kind) {
158-
if (!HasMember(Kind))
159-
MissingMembers.push_back(Kind);
183+
auto RequireMembers = [&](SpecialMemberFunctionKind Kind1,
184+
SpecialMemberFunctionKind Kind2) {
185+
if (AllowImplicitlyDeletedCopyOrMove && HasImplicitDeletedMember(Kind1) &&
186+
HasImplicitDeletedMember(Kind2))
187+
return;
188+
189+
if (!HasMember(Kind1))
190+
MissingMembers.push_back(Kind1);
191+
192+
if (!HasMember(Kind2))
193+
MissingMembers.push_back(Kind2);
160194
};
161195

162196
bool RequireThree =
@@ -180,23 +214,25 @@ void SpecialMemberFunctionsCheck::checkForMissingMembers(
180214
!HasMember(SpecialMemberFunctionKind::NonDefaultDestructor))
181215
MissingMembers.push_back(SpecialMemberFunctionKind::Destructor);
182216

183-
RequireMember(SpecialMemberFunctionKind::CopyConstructor);
184-
RequireMember(SpecialMemberFunctionKind::CopyAssignment);
217+
RequireMembers(SpecialMemberFunctionKind::CopyConstructor,
218+
SpecialMemberFunctionKind::CopyAssignment);
185219
}
186220

187221
if (RequireFive &&
188222
!(AllowMissingMoveFunctionsWhenCopyIsDeleted &&
189223
(IsDeleted(SpecialMemberFunctionKind::CopyConstructor) &&
190224
IsDeleted(SpecialMemberFunctionKind::CopyAssignment)))) {
191225
assert(RequireThree);
192-
RequireMember(SpecialMemberFunctionKind::MoveConstructor);
193-
RequireMember(SpecialMemberFunctionKind::MoveAssignment);
226+
RequireMembers(SpecialMemberFunctionKind::MoveConstructor,
227+
SpecialMemberFunctionKind::MoveAssignment);
194228
}
195229

196230
if (!MissingMembers.empty()) {
197231
llvm::SmallVector<SpecialMemberFunctionKind, 5> DefinedMemberKinds;
198-
llvm::transform(DefinedMembers, std::back_inserter(DefinedMemberKinds),
199-
[](const auto &Data) { return Data.FunctionKind; });
232+
for (const auto &Data : DefinedMembers) {
233+
if (!Data.IsImplicit)
234+
DefinedMemberKinds.push_back(Data.FunctionKind);
235+
}
200236
diag(ID.first, "class '%0' defines %1 but does not define %2")
201237
<< ID.second << cppcoreguidelines::join(DefinedMemberKinds, " and ")
202238
<< cppcoreguidelines::join(MissingMembers, " or ");

‎clang-tools-extra/clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.h

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -30,9 +30,8 @@ class SpecialMemberFunctionsCheck : public ClangTidyCheck {
3030
void registerMatchers(ast_matchers::MatchFinder *Finder) override;
3131
void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
3232
void onEndOfTranslationUnit() override;
33-
std::optional<TraversalKind> getCheckTraversalKind() const override {
34-
return TK_IgnoreUnlessSpelledInSource;
35-
}
33+
std::optional<TraversalKind> getCheckTraversalKind() const override;
34+
3635
enum class SpecialMemberFunctionKind : uint8_t {
3736
Destructor,
3837
DefaultDestructor,
@@ -46,6 +45,7 @@ class SpecialMemberFunctionsCheck : public ClangTidyCheck {
4645
struct SpecialMemberFunctionData {
4746
SpecialMemberFunctionKind FunctionKind;
4847
bool IsDeleted;
48+
bool IsImplicit = false;
4949

5050
bool operator==(const SpecialMemberFunctionData &Other) const {
5151
return (Other.FunctionKind == FunctionKind) &&
@@ -67,6 +67,7 @@ class SpecialMemberFunctionsCheck : public ClangTidyCheck {
6767
const bool AllowMissingMoveFunctions;
6868
const bool AllowSoleDefaultDtor;
6969
const bool AllowMissingMoveFunctionsWhenCopyIsDeleted;
70+
const bool AllowImplicitlyDeletedCopyOrMove;
7071
ClassDefiningSpecialMembersMap ClassWithSpecialMembers;
7172
};
7273

‎clang-tools-extra/docs/ReleaseNotes.rst

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -262,6 +262,12 @@ Changes in existing checks
262262
<clang-tidy/checks/cppcoreguidelines/use-default-member-init>`. Fixed
263263
incorrect hints when using list-initialization.
264264

265+
- Improved :doc:`cppcoreguidelines-special-member-functions
266+
<clang-tidy/checks/cppcoreguidelines/special-member-functions>` check with a
267+
new option `AllowImplicitlyDeletedCopyOrMove`, which removes the requirement
268+
for explicit copy or move special member functions when they are already
269+
implicitly deleted.
270+
265271
- Improved :doc:`google-build-namespaces
266272
<clang-tidy/checks/google/build-namespaces>` check by replacing the local
267273
option `HeaderFileExtensions` by the global option of the same name.

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

Lines changed: 21 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -45,9 +45,10 @@ Options
4545

4646
.. option:: AllowMissingMoveFunctions
4747

48-
When set to `true` (default is `false`), this check doesn't flag classes which define no move
49-
operations at all. It still flags classes which define only one of either
50-
move constructor or move assignment operator. With this option enabled, the following class won't be flagged:
48+
When set to `true` (default is `false`), this check doesn't flag classes
49+
which define no move operations at all. It still flags classes which define
50+
only one of either move constructor or move assignment operator. With this
51+
option enabled, the following class won't be flagged:
5152

5253
.. code-block:: c++
5354

@@ -59,10 +60,11 @@ Options
5960

6061
.. option:: AllowMissingMoveFunctionsWhenCopyIsDeleted
6162

62-
When set to `true` (default is `false`), this check doesn't flag classes which define deleted copy
63-
operations but don't define move operations. This flag is related to Google C++ Style Guide
64-
https://google.github.io/styleguide/cppguide.html#Copyable_Movable_Types. With this option enabled, the
65-
following class won't be flagged:
63+
When set to `true` (default is `false`), this check doesn't flag classes
64+
which define deleted copy operations but don't define move operations. This
65+
flag is related to Google C++ Style Guide `Copyable and Movable Types
66+
<https://google.github.io/styleguide/cppguide.html#Copyable_Movable_Types>`_.
67+
With this option enabled, the following class won't be flagged:
6668

6769
.. code-block:: c++
6870

@@ -71,3 +73,15 @@ Options
7173
A& operator=(const A&) = delete;
7274
~A();
7375
};
76+
77+
.. option:: AllowImplicitlyDeletedCopyOrMove
78+
79+
When set to `true` (default is `false`), this check doesn't flag classes
80+
which implicitly delete copy or move operations.
81+
With this option enabled, the following class won't be flagged:
82+
83+
.. code-block:: c++
84+
85+
struct A : boost::noncopyable {
86+
~A() { std::cout << "dtor\n"; }
87+
};

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

Lines changed: 23 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
// RUN: %check_clang_tidy %s cppcoreguidelines-special-member-functions %t -- -config="{CheckOptions: {cppcoreguidelines-special-member-functions.AllowMissingMoveFunctions: true, cppcoreguidelines-special-member-functions.AllowSoleDefaultDtor: true}}" --
1+
// 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}}" --
22

33
// Don't warn on destructors without definitions, they might be defaulted in another TU.
44
class DeclaresDestructor {
@@ -34,12 +34,13 @@ class DefinesCopyAssignment {
3434
class DefinesMoveConstructor {
3535
DefinesMoveConstructor(DefinesMoveConstructor &&);
3636
};
37-
// 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]
37+
// 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]
3838

3939
class DefinesMoveAssignment {
4040
DefinesMoveAssignment &operator=(DefinesMoveAssignment &&);
4141
};
42-
// 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]
42+
// 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]
43+
4344
class DefinesNothing {
4445
};
4546

@@ -81,3 +82,22 @@ struct TemplateClass {
8182
// This should not cause problems.
8283
TemplateClass<int> InstantiationWithInt;
8384
TemplateClass<double> InstantiationWithDouble;
85+
86+
struct NoCopy
87+
{
88+
NoCopy() = default;
89+
~NoCopy() = default;
90+
91+
NoCopy(const NoCopy&) = delete;
92+
NoCopy(NoCopy&&) = delete;
93+
94+
NoCopy& operator=(const NoCopy&) = delete;
95+
NoCopy& operator=(NoCopy&&) = delete;
96+
};
97+
98+
// 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]
99+
struct NonCopyable : NoCopy
100+
{
101+
NonCopyable() = default;
102+
NonCopyable(const NonCopyable&) = delete;
103+
};

0 commit comments

Comments
 (0)