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

[Alexander Kornienko via cfe-commits]

alexfh added inline comments.

================
Comment at:  clang-tidy/readability/DeletedDefaultCheck.cpp:26
@@ +25,3 @@
+void DeletedDefaultCheck::registerMatchers(MatchFinder *Finder) {
+  const auto NotTemplate = unless(hasAncestor(
+      cxxRecordDecl(::clang::ast_matchers::isTemplateInstantiation())));
----------------
`unless(isInTemplateInstantiation())` is the canonical way to do this.

================
Comment at:  clang-tidy/readability/DeletedDefaultCheck.cpp:30
@@ +29,3 @@
+  // We should actually use isExplicitlyDefaulted, but it does not exist.
+  Finder->addMatcher(
+      cxxConstructorDecl(isDefaultConstructor(), isExplicitlyDefaulted(),
----------------
I think, we need at most two matchers here: one for constructors and one for assignment operators. We could also cram these into a single matcher. I also suggest to combine the `diag()` calls to a single one using `%N` or `%select{}N` format to parametrize the message. Something along the lines of:

  Finder->addMatcher(cxxMethodDecl(isDefaulted(), isDeleted(), unless(isImplicit()), unless(isInTemplateInstantiation()),
      anyOf(isCopyAssignmentOperator(), isMoveAssignmentOperator(), cxxConstructorDecl().bind("ctor"))).bind("method"));

  ...
  const auto *Method = Result.Nodes.getNodeAs<CXXMethodDecl>("method");
  assert(Method != nullptr);
  diag(Method->getLocStart(),
         "this %select{method|constructor}0 is marked '= default' but is actually "
         "implicitly deleted, probably because an instance variable or a base "
         "class is not copyable nor movable; this definition should either be removed "
         "or explicitly marked as '= delete'") << (Result.Nodes.getNodeAs<Decl>("ctor") ? 1 : 0);

If you think that adding "copy", "move" or "default" makes the message any better, this could also be accommodated in this approach.

================
Comment at:  clang-tidy/readability/DeletedDefaultCheck.cpp:31
@@ +30,3 @@
+  Finder->addMatcher(
+      cxxConstructorDecl(isDefaultConstructor(), isExplicitlyDefaulted(),
+                         isDeleted(), NotTemplate)
----------------
Alternatively, you can use `isDefaulted(), unless(isImplicit())`.

================
Comment at:  test/clang-tidy/readability-deleted-default.cpp:13
@@ +12,3 @@
+  MissingEverything() = default;
+  // CHECK-MESSAGES: warning: this default constructor is marked '= default' but is actually implicitly deleted
+  MissingEverything(MissingEverything&& Other) = default;
----------------
Please specify the message completely once.


http://reviews.llvm.org/D18961