[PATCH] D18961: Add a readability-deleted-default clang-tidy check.

[Alexander Kornienko via cfe-commits]

alexfh added a comment.

Thank you for addressing the comments!


================
Comment at:  clang-tidy/readability/DeletedDefaultCheck.cpp:27
@@ +26,3 @@
+  const auto NotTemplate = unless(hasAncestor(
+      cxxRecordDecl(::clang::ast_matchers::isTemplateInstantiation())));
+
----------------
What about this comment?

================
Comment at:  clang-tidy/readability/DeletedDefaultCheck.cpp:32
@@ +31,3 @@
+          .bind("constructor"),
+      this);
+  Finder->addMatcher(cxxMethodDecl(anyOf(isCopyAssignmentOperator(),
----------------
Yes, using standard tools instead of introducing a new one.

================
Comment at:  clang-tidy/readability/DeletedDefaultCheck.cpp:44
@@ +43,3 @@
+      "this %0 is marked '= default' but is actually implicitly deleted, "
+      "probably because %1. You should either remove this definition or "
+      "explicitly mark it as '= delete'";
----------------
A few suggestions to improve the wording:
  * remove "this"
  * replace the period in the end of the first sentence with a semicolon and use a lower-case letter to start the second part.
  * "You should do X" doesn't sound right in warning messages. I'd use a softer option, for example in passive voice: "this definition should either be removed or explicitly marked as '= delete'"

================
Comment at:  clang-tidy/readability/DeletedDefaultCheck.cpp:49
@@ +48,3 @@
+    if (Constructor->isDefaultConstructor()) {
+      diag(Constructor->getLocStart(), Message)
+          << "default constructor"
----------------
If the check() method issues a diagnostic in any case, I'd prefer a slightly different pattern to reduce code duplication a bit more:

  auto Diag = diag(Constructor->getLocStart(), ".....");
  if (x) {
    Diag << "qqq" << "eee";
  }
  ...

================
Comment at:  clang-tidy/readability/DeletedDefaultCheck.cpp:70
@@ +69,3 @@
+          Result.Nodes.getNodeAs<CXXMethodDecl>("assignment")) {
+    const StringRef Explanation = "a base class or an instance variable is not "
+                                  "assignable, e.g. because the latter is "
----------------
nit: No need for this variable.

================
Comment at:  clang-tidy/readability/DeletedDefaultCheck.cpp:72
@@ +71,3 @@
+                                  "assignable, e.g. because the latter is "
+                                  "marked as const";
+    diag(Assignment->getLocStart(), Message)
----------------
"marked 'const'" sounds better to me than "marked as const".


http://reviews.llvm.org/D18961