[clang-tidy] fix segfault in cppcore-guidelines-special-member-functions check

[jbcoe]

Summary

Use a set rather than a vector of defined special member functions so that multiple declarations of the same function are only counted once.

Move some private static member functions into the cpp file.

Run clang-format on header.

[aaron.ballman]

Please add a test case that would have crashed previously, but this patch corrects. With that test case, this patch will LG.

[jbcoe]

Add test that triggers segfault without fix in place.

[aaron.ballman]

clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.cpp

63–64 ↗

(On Diff #66444)

I hadn't noticed this before -- why has this been converted to an unrestricted template? If generality is what you were going for, it should have accepted a range (or a pair of iterators) instead?

[Prazek]

LGTM

[jbcoe]

clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.cpp

63–64 ↗

(On Diff #66444)

It needs to handle SmallSetVector which I can't convert to an ArrayRef.

How do I specify a range?

[aaron.ballman]

clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.cpp

63–64 ↗

(On Diff #66444)

template <typename Iter>
static std::string join(llvm::iterator_range<Iter> R, llvm::StringRef AndOr);

Untested, but you get the idea.

[jbcoe]

static join function is no longer a function template.

[jbcoe]

clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.cpp

63–64 ↗

(On Diff #66541)

iterator_range loses me size, empty and index access. There's a function that gets and ArrayRef from a SmallSetVector (I wonder why there's no implicit conversion defined) so I can change the signature to take ArrayRef.

[aaron.ballman]

LGTM now, thank you for switching back! I was mostly worried about an unrestricted template argument in this case, not the particular form of the change. :-)

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

-//===--- SpecialMemberFunctionsCheck.h - clang-tidy-------------------*- C++ -*-===//
+//===--- 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.
 //
 //===----------------------------------------------------------------------===//
 
enum class SpecialMemberFunctionKind {
     CopyConstructor,
     CopyAssignment,
     MoveConstructor,
     MoveAssignment
   };
 
   using ClassDefId = std::pair<SourceLocation, std::string>;
 
-  using ClassDefiningSpecialMembersMap = llvm::DenseMap<ClassDefId, llvm::SmallVector<SpecialMemberFunctionKind, 5>>;
+  using ClassDefiningSpecialMembersMap =
+      llvm::DenseMap<ClassDefId,
+                     llvm::SmallSetVector<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() {

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

Finder->addMatcher(
                       .bind("move-ctor")),
               has(cxxMethodDecl(isMoveAssignmentOperator(),
                                 unless(isImplicit()))
                       .bind("move-assign"))))
           .bind("class-def"),
       this);
 }
 
-llvm::StringRef SpecialMemberFunctionsCheck::toString(
-    SpecialMemberFunctionsCheck::SpecialMemberFunctionKind K) {
+static llvm::StringRef
+toString(SpecialMemberFunctionsCheck::SpecialMemberFunctionKind K) {
   switch (K) {
-  case SpecialMemberFunctionKind::Destructor:
+  case SpecialMemberFunctionsCheck::SpecialMemberFunctionKind::Destructor:
     return "a destructor";
-  case SpecialMemberFunctionKind::CopyConstructor:
+  case SpecialMemberFunctionsCheck::SpecialMemberFunctionKind::CopyConstructor:
     return "a copy constructor";
-  case SpecialMemberFunctionKind::CopyAssignment:
+  case SpecialMemberFunctionsCheck::SpecialMemberFunctionKind::CopyAssignment:
     return "a copy assignment operator";
-  case SpecialMemberFunctionKind::MoveConstructor:
+  case SpecialMemberFunctionsCheck::SpecialMemberFunctionKind::MoveConstructor:
     return "a move constructor";
-  case SpecialMemberFunctionKind::MoveAssignment:
+  case SpecialMemberFunctionsCheck::SpecialMemberFunctionKind::MoveAssignment:
     return "a move assignment operator";
   }
   llvm_unreachable("Unhandled SpecialMemberFunctionKind");
 }
 
-std::string SpecialMemberFunctionsCheck::join(
-    llvm::ArrayRef<SpecialMemberFunctionKind> SMFS, llvm::StringRef AndOr) {
+static std::string
+join(ArrayRef<SpecialMemberFunctionsCheck::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;
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);
+      ClassWithSpecialMembers[ID].insert(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;
+    const auto &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 ")
+        << C.first.second << join(DefinedSpecialMembers.getArrayRef(), " and ")
         << join(UndefinedSpecialMembers, " or ");
   }
 }
 } // namespace cppcoreguidelines
 } // namespace tidy
 } // namespace clang

clang-tools-extra/trunk/test/clang-tidy/cppcoreguidelines-special-member-functions.cpp

 
 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;