[clang-tidy] Add AllowImplicitlyDeletedCopyOrMove option to cppcoreguidelines-special-member-functions #71683
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-clang-tools-extra @llvm/pr-subscribers-clang-tidy Author: Piotr Zegar (PiotrZSL) ChangesImproved 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 Full diff: https://github.com/llvm/llvm-project/pull/71683.diff 5 Files Affected:
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;
+};
|
5chmidti
approved these changes
Jan 21, 2024
|
Bump. |
…idelines-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.
PiotrZSL
force-pushed
the
62392-clang-tidy-complains-on-implicitly-deleted-special-member-functions
branch
from
ab519d1 to
40d7a61
Compare
PiotrZSL
deleted the
62392-clang-tidy-complains-on-implicitly-deleted-special-member-functions
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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