[clang-tidy] Added options to cppcoreguidelines-special-member-functions check

[fgross]

Summary

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&);
};

[aaron.ballman]

Missing documentation changes for the new options.

clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.cpp

112

Please qualify the find with std::, here and elsewhere.

140–171

Formatting

142

{ should go up a line.

155

Can elide the parens.

169

This feels like a lot of code duplication.

clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.h

51

You should run clang-format over the patch; the & should bind to ID.

[fgross]

Updated documentation, got rid of code duplication.

[fgross]

clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.cpp

112

It's actually llvm::; added namespace and switched to is_contained.

[fgross]

reformatted

[aaron.ballman]

clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.cpp

116

const auto *Dtor

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

23

Examples for each of the options might be nice.

[fgross]

Added examples to options doc.

[fgross]

Diff was missing cppcoreguidelines-special-member-functions-relaxed.cpp...

[aaron.ballman]

LGTM!

[alexfh]

clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.cpp

184

A note on usability: the diagnostic might be more useful, if it had a note pointing to each of the special members mentioned in %1. Just a thought without any relevance to this patch.

[fgross]

No commit access, could someone please take care of this? Thanks!

[aaron.ballman]

I've commit in r297671, thanks!

[malcolm.parsons]

I'd like an AllowDeletedCopyFunctions option that allows move and destructor functions to be missing when copying is disabled.

struct A {
  A(const A&) = delete;
  A& operator=(const A&) = delete;
}

[aaron.ballman]

In D30610#934452, @malcolm.parsons wrote:

I'd like an AllowDeletedCopyFunctions option that allows move and destructor functions to be missing when copying is disabled.

struct A {
  A(const A&) = delete;
  A& operator=(const A&) = delete;
}

Doesn't AllowMissingMoveFunctions do almost that? If not, it should -- that code produces a class that does not declare a move constructor or move assignment operator per [class.copy]p8 and [class.copy.assign]p4, so that would be a "missing move function". Granted, that doesn't handle the dtor case, but I think an AllowMissingDestructor option might be overkill -- the destructor isn't missing, it's implicitly declared as defaulted in that case, but if the C++ Core Guidelines folks want it spelled out explicitly then, it might be worth the option. Have they weighed in on your exception?

[malcolm.parsons]

In D30610#934617, @aaron.ballman wrote:

In D30610#934452, @malcolm.parsons wrote:

I'd like an AllowDeletedCopyFunctions option that allows move and destructor functions to be missing when copying is disabled.

struct A {
  A(const A&) = delete;
  A& operator=(const A&) = delete;
}

Doesn't AllowMissingMoveFunctions do almost that? If not, it should -- that code produces a class that does not declare a move constructor or move assignment operator per [class.copy]p8 and [class.copy.assign]p4, so that would be a "missing move function". Granted, that doesn't handle the dtor case, but I think an AllowMissingDestructor option might be overkill -- the destructor isn't missing, it's implicitly declared as defaulted in that case, but if the C++ Core Guidelines folks want it spelled out explicitly then, it might be worth the option. Have they weighed in on your exception?

The check is about providing a consistent set of special member functions.
If you've written a destructor then you probably need to write copy functions to avoid double free.
If you've defaulted the destructor, as often needed in a base class, then the default copy functions are still valid, so AllowSoleDefaultDtor makes sense.
If you've written your own copy functions then you probably want to write your own move functions to allow moving, so AllowMissingMoveFunctions doesn't make sense.
If you've deleted the copy functions, then you probably don't want the move functions either, so AllowDeletedCopyFunctions would make sense.

I haven't asked the Core Guidelines folks.

[aaron.ballman]

In D30610#934704, @malcolm.parsons wrote:

In D30610#934617, @aaron.ballman wrote:

In D30610#934452, @malcolm.parsons wrote:

I'd like an AllowDeletedCopyFunctions option that allows move and destructor functions to be missing when copying is disabled.

struct A {
  A(const A&) = delete;
  A& operator=(const A&) = delete;
}

Doesn't AllowMissingMoveFunctions do almost that? If not, it should -- that code produces a class that does not declare a move constructor or move assignment operator per [class.copy]p8 and [class.copy.assign]p4, so that would be a "missing move function". Granted, that doesn't handle the dtor case, but I think an AllowMissingDestructor option might be overkill -- the destructor isn't missing, it's implicitly declared as defaulted in that case, but if the C++ Core Guidelines folks want it spelled out explicitly then, it might be worth the option. Have they weighed in on your exception?

The check is about providing a consistent set of special member functions.

Understood.

If you've written a destructor then you probably need to write copy functions to avoid double free.

Sometimes. RAII is a good example where that's not always true. However, in the general case, I agree.

If you've defaulted the destructor, as often needed in a base class, then the default copy functions are still valid, so AllowSoleDefaultDtor makes sense.

Agreed.

If you've written your own copy functions then you probably want to write your own move functions to allow moving, so AllowMissingMoveFunctions doesn't make sense.

I disagree. Move constructors can gracefully degenerate into a copy operation when the copy and move semantics are identical. Think about classes that only contain scalar values as a common example of this. AllowMissingMoveFunctions is the option which serves that common purpose.

If you've deleted the copy functions, then you probably don't want the move functions either, so AllowDeletedCopyFunctions would make sense.

I agree with the premise, but disagree with your conclusion. If you've deleted the copy functions *you don't have the move functions* unless you explicitly spell them out (they are not implicitly generated for you). To that end, I don't think this check should diagnose the lack of move operations in the presence of deleted copy operations. However, I'd like to know what the Core Guidelines people think about this scenario. If the copy operations are explicitly deleted, the primary difference between the default behavior and explicitly deleted move operations is that the diagnostics change from "no such operator" to "explicitly deleted operator". Do they think that's worth the extra typing for the user?

[fgross]

If you've written your own copy functions then you probably want to write your own move functions to allow moving, so AllowMissingMoveFunctions doesn't make sense.

The scenario I had in mind was legacy code which was written back when it still was the "rule of three" instead of the "rule of five". Those classes with defined destructor and copy operations are still perfectly safe, because moving them falls back to copying. They may not be perfectly tuned for performance, but having no move operations is not an indication for some resoure management error. That's why I do think this options makes sense.

I'd like an AllowDeletedCopyFunctions option that allows move and destructor functions to be missing when copying is disabled.

struct A {
  A(const A&) = delete;
  A& operator=(const A&) = delete;
}

My 2 cents on this one: Even with AllowMissingMoveFunctions=1 at least the missing destructor definition should be diagnosed, because it violates the classic rule of three. If you delete your copy operations, you likely have some resources that need to be taken care of in your destructor, so this piece of code would worry me. Better be clear about it and explicitly default the destructor.

[malcolm.parsons]

I have 1000s of warnings from this check.

A lot of them are about google tests:
warning: class 'Foo_Bar_Test' defines a copy constructor and a copy assignment operator but does not define a destructor [cppcoreguidelines-special-member-functions]
I'd like an option to suppress these.

It warns about classes that use boost::noncopyable:
warning: class 'Foo' defines a non-default destructor but does not define a copy constructor or a copy assignment operator [cppcoreguidelines-special-member-functions]
class Foo : boost::noncopyable
I think this is a bug in the check.

In D30610#934862, @fgross wrote:

My 2 cents on this one: Even with AllowMissingMoveFunctions=1 at least the missing destructor definition should be diagnosed, because it violates the classic rule of three. If you delete your copy operations, you likely have some resources that need to be taken care of in your destructor, so this piece of code would worry me. Better be clear about it and explicitly default the destructor.

This is a rule of zero class, but with copying disabled; the resources are just as safe as with a normal rule of zero class.

[aaron.ballman]

In D30610#934891, @malcolm.parsons wrote:

I have 1000s of warnings from this check.

A lot of them are about google tests:
warning: class 'Foo_Bar_Test' defines a copy constructor and a copy assignment operator but does not define a destructor [cppcoreguidelines-special-member-functions]
I'd like an option to suppress these.

It warns about classes that use boost::noncopyable:
warning: class 'Foo' defines a non-default destructor but does not define a copy constructor or a copy assignment operator [cppcoreguidelines-special-member-functions]
class Foo : boost::noncopyable
I think this is a bug in the check.

It is not a bug in the check; the check implements the C++ Core Guideline rule. Hence the comment about discussing it with them.

I think your use cases are compelling and might warrant an option. However, with that in mind combined with AllowMissingMoveFunctions, it makes me think this check should not have any of the options -- it's a sign that your code base isn't ready to comply with C.21 in C++ Core Guidelines. The wording for the guideline is pretty specific about what the authors intend: "Compilers enforce much of this rule and ideally warn about any violation." and its enforcement "(Simple) A class should have a declaration (even a =delete one) for either all or none of the special functions." I'm not saying we shouldn't add this option you're looking for, but I'm still a bit uncomfortable with coming up with option combinations that effectively disable the entire point to a check in a coding standard -- that's why we allow you to disable the check more visibly.

In D30610#934862, @fgross wrote:

My 2 cents on this one: Even with AllowMissingMoveFunctions=1 at least the missing destructor definition should be diagnosed, because it violates the classic rule of three. If you delete your copy operations, you likely have some resources that need to be taken care of in your destructor, so this piece of code would worry me. Better be clear about it and explicitly default the destructor.

This is a rule of zero class, but with copying disabled; the resources are just as safe as with a normal rule of zero class.

Depending on the resource; you might allocate a resource in the constructor, deleted copy (and thus no move) operators, but still want to release the resource in the destructor.

clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.h

 
 /// Checks for classes where some, but not all, of the special member functions
 /// are defined.
 ///
 /// For the user-facing documentation see:
 /// 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,
     MoveAssignment
   };
 
   using ClassDefId = std::pair<SourceLocation, std::string>;
 
   using ClassDefiningSpecialMembersMap =
       llvm::DenseMap<ClassDefId,
                      llvm::SmallVector<SpecialMemberFunctionKind, 5>>;
 
 private:
+  void checkForMissingMembers(
+      const ClassDefId &ID,
+      llvm::ArrayRef<SpecialMemberFunctionKind> DefinedSpecialMembers);
+
+  const bool AllowMissingMoveFunctions;
+  const bool AllowSoleDefaultDtor;
   ClassDefiningSpecialMembersMap ClassWithSpecialMembers;
 };
 
 } // namespace cppcoreguidelines
 } // namespace tidy
 } // namespace clang
 
 namespace llvm {

clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.cpp

 #define DEBUG_TYPE "clang-tidy"
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 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;
   Finder->addMatcher(
       cxxRecordDecl(
           eachOf(
               has(cxxDestructorDecl(unless(isImplicit())).bind("dtor")),
               has(cxxConstructorDecl(isCopyConstructor(), unless(isImplicit()))
Finder->addMatcher(
       this);
 }
 
 static llvm::StringRef
 toString(SpecialMemberFunctionsCheck::SpecialMemberFunctionKind K) {
   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:
     return "a copy assignment operator";
   case SpecialMemberFunctionsCheck::SpecialMemberFunctionKind::MoveConstructor:
     return "a move constructor";
   case SpecialMemberFunctionsCheck::SpecialMemberFunctionKind::MoveAssignment:
     return "a move assignment operator";

 void SpecialMemberFunctionsCheck::check(
     const MatchFinder::MatchResult &Result) {
   const auto *MatchedDecl = Result.Nodes.getNodeAs<CXXRecordDecl>("class-def");
   if (!MatchedDecl)
     return;
 
   ClassDefId ID(MatchedDecl->getLocation(), MatchedDecl->getName());
 
+  auto StoreMember = [this, &ID](SpecialMemberFunctionKind Kind) {
+    llvm::SmallVectorImpl<SpecialMemberFunctionKind> &Members =
+        ClassWithSpecialMembers[ID];
+    if (!llvm::is_contained(Members, Kind))
+      Members.push_back(Kind);
+  };
+
+  if (const 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> DefinedMembers) {
+  llvm::SmallVector<SpecialMemberFunctionKind, 5> MissingMembers;
+
+  auto HasMember = [&](SpecialMemberFunctionKind Kind) {
+    return llvm::is_contained(DefinedMembers, Kind);
+  };
+
+  auto RequireMember = [&](SpecialMemberFunctionKind Kind) {
+    if (!HasMember(Kind))
+      MissingMembers.push_back(Kind);
+  };
+
+  bool RequireThree =
+      HasMember(SpecialMemberFunctionKind::NonDefaultDestructor) ||
+      (!AllowSoleDefaultDtor &&
+       HasMember(SpecialMemberFunctionKind::DefaultDestructor)) ||
+      HasMember(SpecialMemberFunctionKind::CopyConstructor) ||
+      HasMember(SpecialMemberFunctionKind::CopyAssignment) ||
+      HasMember(SpecialMemberFunctionKind::MoveConstructor) ||
+      HasMember(SpecialMemberFunctionKind::MoveAssignment);
 
-    if (DefinedSpecialMembers.size() == AllSpecialMembers.size())
-      continue;
+  bool RequireFive = (!AllowMissingMoveFunctions && RequireThree &&
+                      getLangOpts().CPlusPlus11) ||
+                     HasMember(SpecialMemberFunctionKind::MoveConstructor) ||
+                     HasMember(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 (RequireThree) {
+    if (!HasMember(SpecialMemberFunctionKind::DefaultDestructor) &&
+        !HasMember(SpecialMemberFunctionKind::NonDefaultDestructor))
+      MissingMembers.push_back(SpecialMemberFunctionKind::Destructor);
+
+    RequireMember(SpecialMemberFunctionKind::CopyConstructor);
+    RequireMember(SpecialMemberFunctionKind::CopyAssignment);
   }
+
+  if (RequireFive) {
+    assert(RequireThree);
+    RequireMember(SpecialMemberFunctionKind::MoveConstructor);
+    RequireMember(SpecialMemberFunctionKind::MoveAssignment);
   }
+
+  if (!MissingMembers.empty())
+    diag(ID.first, "class '%0' defines %1 but does not define %2")
+        << ID.second << join(DefinedMembers, " and ")
+        << join(MissingMembers, " or ");
+}
+
 } // namespace cppcoreguidelines
 } // namespace tidy
 } // namespace clang

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

 
 Note that defining a function with ``= delete`` is considered to be a
 definition.
 
 This rule is part of the "Constructors, assignments, and destructors" profile of the C++ Core
 Guidelines, corresponding to rule C.21. See
 
 https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#c21-if-you-define-or-delete-any-default-operation-define-or-delete-them-all.
+
+Options
+-------
+
+.. option:: AllowSoleDefaultDtor
+
+   When set to `1` (default is `0`), this check doesn't flag classes with a sole, explicitly
+   defaulted destructor. An example for such a class is:
+   
+   .. code-block:: c++
+   
+     struct A {
+       virtual ~A() = default;
+     };
+   
+.. option:: AllowMissingMoveFunctions
+
+   When set to `1` (default is `0`), 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++
+   
+     struct A {
+       A(const A&);
+       A& operator=(const A&);
+       ~A();
+     }

test/clang-tidy/cppcoreguidelines-special-member-functions-cxx-03.cpp

 // RUN: %check_clang_tidy %s cppcoreguidelines-special-member-functions %t -- -- -std=c++03
 
 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 &);
 };
 // 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 &);

test/clang-tidy/cppcoreguidelines-special-member-functions.cpp

 // RUN: %check_clang_tidy %s cppcoreguidelines-special-member-functions %t
 
 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 &);
 };
 // CHECK-MESSAGES: [[@LINE-3]]:7: warning: class 'DefinesCopyConstructor' defines a copy constructor but does not define a destructor, a copy assignment operator, a move constructor or a move assignment operator [cppcoreguidelines-special-member-functions]
 
 class DefinesCopyAssignment {
   DefinesCopyAssignment &operator=(const DefinesCopyAssignment &);