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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
