[PATCH] D22513: [clang-tidy] add check cppcoreguidelines-special-member-functions

[Aaron Ballman via cfe-commits]

aaron.ballman added inline comments.

================
Comment at: clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.cpp:62
@@ +61,3 @@
+std::string
+SpecialMemberFunctionsCheck::join(llvm::ArrayRef<SpecialMemberFunctionKind> SMFS,
+                             llvm::StringRef AndOr) {
----------------
I think this join function can maybe be replaced by a call to llvm::join() from StringExtras.h?

================
Comment at: clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.cpp:64
@@ +63,3 @@
+                             llvm::StringRef AndOr) {
+  assert(!SMFS.empty());
+  std::string Buffer;
----------------
It's good to put an && along with some explanatory text for debugging if this assertion ever triggers.

================
Comment at: clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.cpp:73
@@ +72,3 @@
+  }
+  if (LastIndex != 0) {
+    Stream << AndOr << toString(SMFS[LastIndex]);
----------------
Elide braces per usual style conventions.

================
Comment at: clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.cpp:94
@@ +93,3 @@
+
+  for (const auto& KV : Matchers)
+    if (Result.Nodes.getNodeAs<CXXMethodDecl>(KV.first))
----------------
The & should bind to KV rather than auto (clang-format should handle this for you).

================
Comment at: clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.h:42-44
@@ +41,5 @@
+
+  using ClassDefId = std::pair<SourceLocation, std::string>;
+
+  using ClassDefiningSpecialMembersMap = llvm::DenseMap<ClassDefId, llvm::SmallVector<SpecialMemberFunctionKind, 5>>;
+
----------------
Do these need to be public?

================
Comment at: clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.h:62-63
@@ +61,4 @@
+/// 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 <>
----------------
Any reason why not to do this as part of this patch?


https://reviews.llvm.org/D22513