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

[Alex Pilkiewicz via cfe-commits]

pilki added inline comments.

================
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(),
----------------
alexfh wrote:
> 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.
I reduced to two matchers and used a common template for the error message. Tell me if it's ok.

================
Comment at:  clang-tidy/readability/DeletedDefaultCheck.cpp:31
@@ +30,3 @@
+  Finder->addMatcher(
+      cxxConstructorDecl(isDefaultConstructor(), isExplicitlyDefaulted(),
+                         isDeleted(), NotTemplate)
----------------
alexfh wrote:
> Alternatively, you can use `isDefaulted(), unless(isImplicit())`.
What is the advantage? Not writing my own matcher?


http://reviews.llvm.org/D18961