[clang-tidy] add check cppcoreguidelines-special-member-functions
authorJonathan Coe <jbcoe@me.com>
Sat, 30 Jul 2016 08:58:54 +0000 (08:58 +0000)
committerJonathan Coe <jbcoe@me.com>
Sat, 30 Jul 2016 08:58:54 +0000 (08:58 +0000)
Summary:
Check for classes that violate the rule of five and zero as specified in CppCoreGuidelines:

"If a class defines or deletes a default operation then it should define or delete them all."

https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#c21-if-you-define-or-delete-any-default-operation-define-or-delete-them-all.

Reviewers: alexfh, sbenza, aaron.ballman

Subscribers: Prazek, Eugene.Zelenko, cfe-commits, ericLemanissier, nemanjai

Projects: #clang-tools-extra

Differential Revision: https://reviews.llvm.org/D22513

llvm-svn: 277262

clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt
clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
clang-tools-extra/clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.cpp [new file with mode: 0644]
clang-tools-extra/clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.h [new file with mode: 0644]
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-special-member-functions.rst [new file with mode: 0644]
clang-tools-extra/docs/clang-tidy/checks/list.rst
clang-tools-extra/test/clang-tidy/cppcoreguidelines-special-member-functions-cxx-03.cpp [new file with mode: 0644]
clang-tools-extra/test/clang-tidy/cppcoreguidelines-special-member-functions.cpp [new file with mode: 0644]

index 77f0646..62d9a8a 100644 (file)
@@ -13,6 +13,7 @@ add_clang_library(clangTidyCppCoreGuidelinesModule
   ProTypeStaticCastDowncastCheck.cpp
   ProTypeUnionAccessCheck.cpp
   ProTypeVarargCheck.cpp
+  SpecialMemberFunctionsCheck.cpp
   SlicingCheck.cpp
 
   LINK_LIBS
index 608e2ee..dc9a7f3 100644 (file)
@@ -22,6 +22,7 @@
 #include "ProTypeStaticCastDowncastCheck.h"
 #include "ProTypeUnionAccessCheck.h"
 #include "ProTypeVarargCheck.h"
+#include "SpecialMemberFunctionsCheck.h"
 #include "SlicingCheck.h"
 
 namespace clang {
@@ -54,6 +55,8 @@ public:
         "cppcoreguidelines-pro-type-union-access");
     CheckFactories.registerCheck<ProTypeVarargCheck>(
         "cppcoreguidelines-pro-type-vararg");
+    CheckFactories.registerCheck<SpecialMemberFunctionsCheck>(
+        "cppcoreguidelines-special-member-functions");
     CheckFactories.registerCheck<SlicingCheck>(
         "cppcoreguidelines-slicing");
     CheckFactories.registerCheck<misc::UnconventionalAssignOperatorCheck>(
diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.cpp b/clang-tools-extra/clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.cpp
new file mode 100644 (file)
index 0000000..9a0ffa9
--- /dev/null
@@ -0,0 +1,133 @@
+//===--- SpecialMemberFunctionsCheck.cpp - clang-tidy----------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "SpecialMemberFunctionsCheck.h"
+
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "llvm/ADT/DenseMapInfo.h"
+#include "llvm/ADT/StringExtras.h"
+
+#define DEBUG_TYPE "clang-tidy"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace cppcoreguidelines {
+
+void SpecialMemberFunctionsCheck::registerMatchers(MatchFinder *Finder) {
+  if (!getLangOpts().CPlusPlus)
+    return;
+  Finder->addMatcher(
+      cxxRecordDecl(
+          eachOf(
+              has(cxxDestructorDecl(unless(isImplicit())).bind("dtor")),
+              has(cxxConstructorDecl(isCopyConstructor(), unless(isImplicit()))
+                      .bind("copy-ctor")),
+              has(cxxMethodDecl(isCopyAssignmentOperator(),
+                                unless(isImplicit()))
+                      .bind("copy-assign")),
+              has(cxxConstructorDecl(isMoveConstructor(), unless(isImplicit()))
+                      .bind("move-ctor")),
+              has(cxxMethodDecl(isMoveAssignmentOperator(),
+                                unless(isImplicit()))
+                      .bind("move-assign"))))
+          .bind("class-def"),
+      this);
+}
+
+llvm::StringRef SpecialMemberFunctionsCheck::toString(
+    SpecialMemberFunctionsCheck::SpecialMemberFunctionKind K) {
+  switch (K) {
+  case SpecialMemberFunctionKind::Destructor:
+    return "a destructor";
+  case SpecialMemberFunctionKind::CopyConstructor:
+    return "a copy constructor";
+  case SpecialMemberFunctionKind::CopyAssignment:
+    return "a copy assignment operator";
+  case SpecialMemberFunctionKind::MoveConstructor:
+    return "a move constructor";
+  case SpecialMemberFunctionKind::MoveAssignment:
+    return "a move assignment operator";
+  }
+  llvm_unreachable("Unhandled SpecialMemberFunctionKind");
+}
+
+std::string SpecialMemberFunctionsCheck::join(
+    llvm::ArrayRef<SpecialMemberFunctionKind> SMFS, llvm::StringRef AndOr) {
+
+  assert(!SMFS.empty() &&
+         "List of defined or undefined members should never be empty.");
+  std::string Buffer;
+  llvm::raw_string_ostream Stream(Buffer);
+
+  Stream << toString(SMFS[0]);
+  size_t LastIndex = SMFS.size() - 1;
+  for (size_t i = 1; i < LastIndex; ++i) {
+    Stream << ", " << toString(SMFS[i]);
+  }
+  if (LastIndex != 0) {
+    Stream << AndOr << toString(SMFS[LastIndex]);
+  }
+  return Stream.str();
+}
+
+void SpecialMemberFunctionsCheck::check(
+    const MatchFinder::MatchResult &Result) {
+  const CXXRecordDecl *MatchedDecl =
+      Result.Nodes.getNodeAs<CXXRecordDecl>("class-def");
+  if (!MatchedDecl)
+    return;
+
+  ClassDefId ID(MatchedDecl->getLocation(), MatchedDecl->getName());
+
+  std::initializer_list<std::pair<std::string, SpecialMemberFunctionKind>>
+      Matchers = {{"dtor", SpecialMemberFunctionKind::Destructor},
+                  {"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))
+      ClassWithSpecialMembers[ID].push_back(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) {
+    ArrayRef<SpecialMemberFunctionKind> DefinedSpecialMembers = C.second;
+
+    if (DefinedSpecialMembers.size() == AllSpecialMembers.size())
+      continue;
+
+    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 ");
+  }
+}
+} // namespace cppcoreguidelines
+} // namespace tidy
+} // namespace clang
diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.h b/clang-tools-extra/clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.h
new file mode 100644 (file)
index 0000000..6218535
--- /dev/null
@@ -0,0 +1,101 @@
+//===--- SpecialMemberFunctionsCheck.h - clang-tidy-------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_SPECIAL_MEMBER_FUNCTIONS_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_SPECIAL_MEMBER_FUNCTIONS_H
+
+#include "../ClangTidy.h"
+
+#include "llvm/ADT/DenseMapInfo.h"
+
+namespace clang {
+namespace tidy {
+namespace cppcoreguidelines {
+
+/// 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) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+  void onEndOfTranslationUnit() override;
+  
+  enum class SpecialMemberFunctionKind {
+    Destructor,
+    CopyConstructor,
+    CopyAssignment,
+    MoveConstructor,
+    MoveAssignment
+  };
+
+  using ClassDefId = std::pair<SourceLocation, std::string>;
+
+  using ClassDefiningSpecialMembersMap = llvm::DenseMap<ClassDefId, llvm::SmallVector<SpecialMemberFunctionKind, 5>>;
+
+private:
+  
+  static llvm::StringRef toString(SpecialMemberFunctionKind K);
+
+  static std::string join(llvm::ArrayRef<SpecialMemberFunctionKind> SMFS,
+                          llvm::StringRef AndOr);
+  
+  ClassDefiningSpecialMembersMap ClassWithSpecialMembers;
+};
+
+} // namespace cppcoreguidelines
+} // namespace tidy
+} // namespace clang
+
+namespace llvm {
+/// Specialisation of DenseMapInfo to allow ClassDefId objects in DenseMaps
+/// FIXME: Move this to the corresponding cpp file as is done for
+/// clang-tidy/readability/IdentifierNamingCheck.cpp. 
+template <>
+struct DenseMapInfo<
+    clang::tidy::cppcoreguidelines::SpecialMemberFunctionsCheck::ClassDefId> {
+  using ClassDefId =
+    clang::tidy::cppcoreguidelines::SpecialMemberFunctionsCheck::ClassDefId;
+
+  static inline ClassDefId getEmptyKey() {
+    return ClassDefId(
+        clang::SourceLocation::getFromRawEncoding(static_cast<unsigned>(-1)),
+        "EMPTY");
+  }
+
+  static inline ClassDefId getTombstoneKey() {
+    return ClassDefId(
+        clang::SourceLocation::getFromRawEncoding(static_cast<unsigned>(-2)),
+        "TOMBSTONE");
+  }
+
+  static unsigned getHashValue(ClassDefId Val) {
+    assert(Val != getEmptyKey() && "Cannot hash the empty key!");
+    assert(Val != getTombstoneKey() && "Cannot hash the tombstone key!");
+
+    std::hash<ClassDefId::second_type> SecondHash;
+    return Val.first.getRawEncoding() + SecondHash(Val.second);
+  }
+
+  static bool isEqual(ClassDefId LHS, ClassDefId RHS) {
+    if (RHS == getEmptyKey())
+      return LHS == getEmptyKey();
+    if (RHS == getTombstoneKey())
+      return LHS == getTombstoneKey();
+    return LHS == RHS;
+  }
+};
+
+} // namespace llvm
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_SPECIAL_MEMBER_FUNCTIONS_H
index e2e9bdb..03d1c0f 100644 (file)
@@ -64,6 +64,11 @@ Improvements to clang-tidy
 
   Flags slicing of member variables or vtable.
 
+- New `cppcoreguidelines-special-member-functions
+  <http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-special-member-functions.html>`_ check
+
+  Flags classes where some, but not all, special member functions are user-defined.
+
 Improvements to include-fixer
 -----------------------------
 
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
new file mode 100644 (file)
index 0000000..1e4d017
--- /dev/null
@@ -0,0 +1,21 @@
+.. title:: clang-tidy - cppcoreguidelines-special-member-functions
+
+cppcoreguidelines-special-member-functions
+==========================================
+
+The check finds classes where some but not all of the special member functions
+are defined.
+
+By default the compiler defines a copy constructor, copy assignment operator,
+move constructor, move assignment operator and destructor. The default can be
+supressed by explicit user-definitions. The relationship between which
+functions will be supressed by definitions of other functions is complicated
+and it is advised that all five are defaulted or explicitly defined.
+
+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.
index e8fc21e..461fae3 100644 (file)
@@ -30,6 +30,7 @@ Clang-Tidy Checks
    cppcoreguidelines-pro-type-union-access
    cppcoreguidelines-pro-type-vararg
    cppcoreguidelines-slicing
+   cppcoreguidelines-special-member-functions
    google-build-explicit-make-pair
    google-build-namespaces
    google-build-using-namespace
diff --git a/clang-tools-extra/test/clang-tidy/cppcoreguidelines-special-member-functions-cxx-03.cpp b/clang-tools-extra/test/clang-tidy/cppcoreguidelines-special-member-functions-cxx-03.cpp
new file mode 100644 (file)
index 0000000..ca55f50
--- /dev/null
@@ -0,0 +1,26 @@
+// 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]
+
+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 DefinesNothing {
+};
+
+class DefinesEverything {
+  DefinesEverything(const DefinesEverything &);
+  DefinesEverything &operator=(const DefinesEverything &);
+  ~DefinesEverything();
+};
+
diff --git a/clang-tools-extra/test/clang-tidy/cppcoreguidelines-special-member-functions.cpp b/clang-tools-extra/test/clang-tidy/cppcoreguidelines-special-member-functions.cpp
new file mode 100644 (file)
index 0000000..0eee919
--- /dev/null
@@ -0,0 +1,52 @@
+// 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]
+
+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 &);
+};
+// CHECK-MESSAGES: [[@LINE-3]]:7: warning: class 'DefinesCopyAssignment' defines a copy assignment operator but does not define a destructor, a copy constructor, a move constructor or a move assignment operator [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;
+};