fgross created this revision.
Herald added subscribers: JDevlieghere, nemanjai.
Added two options to cppcoreguidelines-special-member-functions to allow a more
relaxed checking.
With 'AllowSoleDefaultDtor' set to 1, classes with explicitly defaulted
destructors but no other special members are not flagged anymore:
class foo {
// typical scenario: defaulted virtual destructor in base classes
virtual ~foo() = default;
};
With 'AllowMissingMoveFunctions' set to 1, classes with no move functions at
all are not flagged anymore:
class bar{
~bar();
bar(const bar&);
bar& operator=(const bar&);
};
https://reviews.llvm.org/D30610
Files:
clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.cpp
clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.h
test/clang-tidy/cppcoreguidelines-special-member-functions-cxx-03.cpp
test/clang-tidy/cppcoreguidelines-special-member-functions-relaxed.cpp
test/clang-tidy/cppcoreguidelines-special-member-functions.cpp
Index: test/clang-tidy/cppcoreguidelines-special-member-functions.cpp
===================================================================
--- test/clang-tidy/cppcoreguidelines-special-member-functions.cpp
+++ test/clang-tidy/cppcoreguidelines-special-member-functions.cpp
@@ -3,7 +3,12 @@
class DefinesDestructor {
~DefinesDestructor();
};
-// 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]
+// 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]
+
+class DefinesDefaultedDestructor {
+ ~DefinesDefaultedDestructor() = default;
+};
+// CHECK-MESSAGES: [[@LINE-3]]:7: warning: class 'DefinesDefaultedDestructor' defines a 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]
class DefinesCopyConstructor {
DefinesCopyConstructor(const DefinesCopyConstructor &);
Index: test/clang-tidy/cppcoreguidelines-special-member-functions-relaxed.cpp
===================================================================
--- test/clang-tidy/cppcoreguidelines-special-member-functions-relaxed.cpp
+++ test/clang-tidy/cppcoreguidelines-special-member-functions-relaxed.cpp
@@ -0,0 +1,71 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-special-member-functions %t -- -config="{CheckOptions: [{key: cppcoreguidelines-special-member-functions.AllowMissingMoveFunctions, value: 1}, {key: cppcoreguidelines-special-member-functions.AllowSoleDefaultDtor, value: 1}]}" --
+
+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]
+
+class DefinesDefaultedDestructor {
+ ~DefinesDefaultedDestructor() = default;
+};
+
+class DefinesCopyConstructor {
+ DefinesCopyConstructor(const DefinesCopyConstructor &);
+};
+// CHECK-MESSAGES: [[@LINE-3]]:7: warning: class 'DefinesCopyConstructor' defines a copy constructor but does not define a destructor or a copy assignment operator [cppcoreguidelines-special-member-functions]
+
+class DefinesCopyAssignment {
+ DefinesCopyAssignment &operator=(const DefinesCopyAssignment &);
+};
+// CHECK-MESSAGES: [[@LINE-3]]:7: warning: class 'DefinesCopyAssignment' defines a copy assignment operator but does not define a destructor or a copy constructor [cppcoreguidelines-special-member-functions]
+
+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]
+
+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]
+class DefinesNothing {
+};
+
+class DefinesEverything {
+ DefinesEverything(const DefinesEverything &);
+ DefinesEverything &operator=(const DefinesEverything &);
+ DefinesEverything(DefinesEverything &&);
+ DefinesEverything &operator=(DefinesEverything &&);
+ ~DefinesEverything();
+};
+
+class DeletesEverything {
+ DeletesEverything(const DeletesEverything &) = delete;
+ DeletesEverything &operator=(const DeletesEverything &) = delete;
+ DeletesEverything(DeletesEverything &&) = delete;
+ DeletesEverything &operator=(DeletesEverything &&) = delete;
+ ~DeletesEverything() = delete;
+};
+
+class DeletesCopyDefaultsMove {
+ DeletesCopyDefaultsMove(const DeletesCopyDefaultsMove &) = delete;
+ DeletesCopyDefaultsMove &operator=(const DeletesCopyDefaultsMove &) = delete;
+ DeletesCopyDefaultsMove(DeletesCopyDefaultsMove &&) = default;
+ DeletesCopyDefaultsMove &operator=(DeletesCopyDefaultsMove &&) = default;
+ ~DeletesCopyDefaultsMove() = default;
+};
+
+template <typename T>
+struct TemplateClass {
+ TemplateClass() = default;
+ TemplateClass(const TemplateClass &);
+ TemplateClass &operator=(const TemplateClass &);
+ TemplateClass(TemplateClass &&);
+ TemplateClass &operator=(TemplateClass &&);
+ ~TemplateClass();
+};
+
+// Multiple instantiations of a class template will trigger multiple matches for defined special members.
+// This should not cause problems.
+TemplateClass<int> InstantiationWithInt;
+TemplateClass<double> InstantiationWithDouble;
Index: test/clang-tidy/cppcoreguidelines-special-member-functions-cxx-03.cpp
===================================================================
--- test/clang-tidy/cppcoreguidelines-special-member-functions-cxx-03.cpp
+++ test/clang-tidy/cppcoreguidelines-special-member-functions-cxx-03.cpp
@@ -3,7 +3,7 @@
class DefinesDestructor {
~DefinesDestructor();
};
-// 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]
+// 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]
class DefinesCopyConstructor {
DefinesCopyConstructor(const DefinesCopyConstructor &);
Index: clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.h
===================================================================
--- clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.h
+++ clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.h
@@ -25,14 +25,16 @@
/// http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-special-member-functions.html
class SpecialMemberFunctionsCheck : public ClangTidyCheck {
public:
- SpecialMemberFunctionsCheck(StringRef Name, ClangTidyContext *Context)
- : ClangTidyCheck(Name, Context) {}
+ SpecialMemberFunctionsCheck(StringRef Name, ClangTidyContext *Context);
+ void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
void registerMatchers(ast_matchers::MatchFinder *Finder) override;
void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
void onEndOfTranslationUnit() override;
enum class SpecialMemberFunctionKind : uint8_t {
Destructor,
+ DefaultDestructor,
+ NonDefaultDestructor,
CopyConstructor,
CopyAssignment,
MoveConstructor,
@@ -46,6 +48,11 @@
llvm::SmallVector<SpecialMemberFunctionKind, 5>>;
private:
+ void checkForMissingMembers(const ClassDefId& ID,
+ llvm::ArrayRef<SpecialMemberFunctionKind> DefinedSpecialMembers);
+
+ const bool AllowMissingMoveFunctions;
+ const bool AllowSoleDefaultDtor;
ClassDefiningSpecialMembersMap ClassWithSpecialMembers;
};
Index: clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.cpp
===================================================================
--- clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.cpp
+++ clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.cpp
@@ -22,6 +22,18 @@
namespace tidy {
namespace cppcoreguidelines {
+SpecialMemberFunctionsCheck::SpecialMemberFunctionsCheck(
+ StringRef Name, ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context),
+ AllowMissingMoveFunctions(Options.get("AllowMissingMoveFunctions", 0)),
+ AllowSoleDefaultDtor(Options.get("AllowSoleDefaultDtor", 0)) {}
+
+void SpecialMemberFunctionsCheck::storeOptions(
+ ClangTidyOptions::OptionMap &Opts) {
+ Options.store(Opts, "AllowMissingMoveFunctions", AllowMissingMoveFunctions);
+ Options.store(Opts, "AllowSoleDefaultDtor", AllowSoleDefaultDtor);
+}
+
void SpecialMemberFunctionsCheck::registerMatchers(MatchFinder *Finder) {
if (!getLangOpts().CPlusPlus)
return;
@@ -48,6 +60,10 @@
switch (K) {
case SpecialMemberFunctionsCheck::SpecialMemberFunctionKind::Destructor:
return "a destructor";
+ case SpecialMemberFunctionsCheck::SpecialMemberFunctionKind::DefaultDestructor:
+ return "a default destructor";
+ case SpecialMemberFunctionsCheck::SpecialMemberFunctionKind::NonDefaultDestructor:
+ return "a non-default destructor";
case SpecialMemberFunctionsCheck::SpecialMemberFunctionKind::CopyConstructor:
return "a copy constructor";
case SpecialMemberFunctionsCheck::SpecialMemberFunctionKind::CopyAssignment:
@@ -88,51 +104,110 @@
ClassDefId ID(MatchedDecl->getLocation(), MatchedDecl->getName());
+ auto StoreMember = [this, &ID](SpecialMemberFunctionKind Kind) {
+ llvm::SmallVectorImpl<SpecialMemberFunctionKind> &Members =
+ ClassWithSpecialMembers[ID];
+ if (find(Members, Kind) == Members.end())
+ Members.push_back(Kind);
+ };
+
+ if (auto Dtor = Result.Nodes.getNodeAs<CXXMethodDecl>("dtor")) {
+ StoreMember(Dtor->isDefaulted()
+ ? SpecialMemberFunctionKind::DefaultDestructor
+ : SpecialMemberFunctionKind::NonDefaultDestructor);
+ }
+
std::initializer_list<std::pair<std::string, SpecialMemberFunctionKind>>
- Matchers = {{"dtor", SpecialMemberFunctionKind::Destructor},
- {"copy-ctor", SpecialMemberFunctionKind::CopyConstructor},
+ Matchers = {{"copy-ctor", SpecialMemberFunctionKind::CopyConstructor},
{"copy-assign", SpecialMemberFunctionKind::CopyAssignment},
{"move-ctor", SpecialMemberFunctionKind::MoveConstructor},
{"move-assign", SpecialMemberFunctionKind::MoveAssignment}};
for (const auto &KV : Matchers)
if (Result.Nodes.getNodeAs<CXXMethodDecl>(KV.first)) {
- SpecialMemberFunctionKind Kind = KV.second;
- llvm::SmallVectorImpl<SpecialMemberFunctionKind> &Members =
- ClassWithSpecialMembers[ID];
- if (find(Members, Kind) == Members.end())
- Members.push_back(Kind);
+ StoreMember(KV.second);
}
}
void SpecialMemberFunctionsCheck::onEndOfTranslationUnit() {
- llvm::SmallVector<SpecialMemberFunctionKind, 5> AllSpecialMembers = {
- SpecialMemberFunctionKind::Destructor,
- SpecialMemberFunctionKind::CopyConstructor,
- SpecialMemberFunctionKind::CopyAssignment};
-
- if (getLangOpts().CPlusPlus11) {
- AllSpecialMembers.push_back(SpecialMemberFunctionKind::MoveConstructor);
- AllSpecialMembers.push_back(SpecialMemberFunctionKind::MoveAssignment);
+ for (const auto &C : ClassWithSpecialMembers) {
+ checkForMissingMembers(C.first, C.second);
}
+}
- for (const auto &C : ClassWithSpecialMembers) {
- const auto &DefinedSpecialMembers = C.second;
+void SpecialMemberFunctionsCheck::checkForMissingMembers(const ClassDefId& ID,
+ llvm::ArrayRef<SpecialMemberFunctionKind> DefinedSpecialMembers)
+{
+ llvm::SmallVector<SpecialMemberFunctionKind, 5> MissingSpecialMembers;
+
+ auto HasMember = [&](SpecialMemberFunctionKind Kind) {
+ return find(DefinedSpecialMembers, Kind) != DefinedSpecialMembers.end();
+ };
+
+ auto RequireMember = [&](SpecialMemberFunctionKind Kind) {
+ if (!HasMember(Kind))
+ if (find(MissingSpecialMembers, Kind) == MissingSpecialMembers.end())
+ MissingSpecialMembers.push_back(Kind);
+ };
+
+ auto RequireDtor = [&]() {
+ if (!HasMember(SpecialMemberFunctionKind::DefaultDestructor) &&
+ !HasMember(SpecialMemberFunctionKind::NonDefaultDestructor)) {
+ if (find(MissingSpecialMembers, SpecialMemberFunctionKind::Destructor) ==
+ MissingSpecialMembers.end())
+ MissingSpecialMembers.push_back(SpecialMemberFunctionKind::Destructor);
+ }
+ };
+
+ if (HasMember(SpecialMemberFunctionKind::NonDefaultDestructor) ||
+ (!AllowSoleDefaultDtor &&
+ HasMember(SpecialMemberFunctionKind::DefaultDestructor))) {
+ RequireMember(SpecialMemberFunctionKind::CopyConstructor);
+ RequireMember(SpecialMemberFunctionKind::CopyAssignment);
+ if (!AllowMissingMoveFunctions && getLangOpts().CPlusPlus11) {
+ RequireMember(SpecialMemberFunctionKind::MoveConstructor);
+ RequireMember(SpecialMemberFunctionKind::MoveAssignment);
+ }
+ }
+
+ if (HasMember(SpecialMemberFunctionKind::CopyConstructor)) {
+ RequireDtor();
+ RequireMember(SpecialMemberFunctionKind::CopyAssignment);
+ if (!AllowMissingMoveFunctions && getLangOpts().CPlusPlus11) {
+ RequireMember(SpecialMemberFunctionKind::MoveConstructor);
+ RequireMember(SpecialMemberFunctionKind::MoveAssignment);
+ }
+ }
+
+ if (HasMember(SpecialMemberFunctionKind::CopyAssignment)) {
+ RequireDtor();
+ RequireMember(SpecialMemberFunctionKind::CopyConstructor);
+ if (!AllowMissingMoveFunctions && getLangOpts().CPlusPlus11) {
+ RequireMember(SpecialMemberFunctionKind::MoveConstructor);
+ RequireMember(SpecialMemberFunctionKind::MoveAssignment);
+ }
+ }
- if (DefinedSpecialMembers.size() == AllSpecialMembers.size())
- continue;
+ if (HasMember(SpecialMemberFunctionKind::MoveConstructor)) {
+ RequireDtor();
+ RequireMember(SpecialMemberFunctionKind::CopyConstructor);
+ RequireMember(SpecialMemberFunctionKind::CopyAssignment);
+ RequireMember(SpecialMemberFunctionKind::MoveAssignment);
+ }
- llvm::SmallVector<SpecialMemberFunctionKind, 5> UndefinedSpecialMembers;
- std::set_difference(AllSpecialMembers.begin(), AllSpecialMembers.end(),
- DefinedSpecialMembers.begin(),
- DefinedSpecialMembers.end(),
- std::back_inserter(UndefinedSpecialMembers));
-
- diag(C.first.first, "class '%0' defines %1 but does not define %2")
- << C.first.second << join(DefinedSpecialMembers, " and ")
- << join(UndefinedSpecialMembers, " or ");
+ if (HasMember(SpecialMemberFunctionKind::MoveAssignment)) {
+ RequireDtor();
+ RequireMember(SpecialMemberFunctionKind::CopyConstructor);
+ RequireMember(SpecialMemberFunctionKind::CopyAssignment);
+ RequireMember(SpecialMemberFunctionKind::MoveConstructor);
}
+
+ if (!MissingSpecialMembers.empty())
+ diag(ID.first, "class '%0' defines %1 but does not define %2")
+ << ID.second << join(DefinedSpecialMembers, " and ")
+ << join(MissingSpecialMembers, " or ");
}
+
} // namespace cppcoreguidelines
} // namespace tidy
} // namespace clang
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
