aaron.ballman added inline comments.
================
Comment at: clang-tidy/modernize/UseEqualsDeleteCheck.cpp:29
+ cxxMethodDecl(
+ anyOf(isCopyAssignmentOperator(), isMoveAssignmentOperator())),
+ cxxDestructorDecl()));
----------------
How about a conversion operator, like `operator bool()`? You'll sometimes see
that one declared privately for similar reasons.
================
Comment at: clang-tidy/modernize/UseEqualsDeleteCheck.cpp:37
+ // Ensure that all methods except private special member
+ // functions are defined
+ hasParent(cxxRecordDecl(hasMethod(unless(
----------------
Missing a full stop at the end of the comment.
================
Comment at: clang-tidy/modernize/UseEqualsDeleteCheck.cpp:46
+void UseEqualsDeleteCheck::check(const MatchFinder::MatchResult &Result) {
+ const auto *SpecialFunction =
+ Result.Nodes.getNodeAs<CXXMethodDecl>("SpecialFunction");
----------------
Should rename this to not hide the global `SpecialFunction` object, and use the
global `SpecialFunction` object in place of the string literal here.
Alternatively, leave this name alone and remove the global variable and just
use the string literal; either is fine.
================
Comment at: clang-tidy/modernize/UseEqualsDeleteCheck.cpp:52
+ diag(SpecialFunction->getLocation(),
+ "use '= delete' to prevent a default special member function")
+ << FixItHint::CreateInsertion(EndLoc, " = delete");
----------------
This diagnostic isn't very clear to me -- what does it mean to "prevent" a
default special member function?
The fixit for this is also somewhat unsatisfying as this takes a private,
not-defined function and turns it into a private, deleted function. That's a
very small win, because it only impacts code which could access the special
member function in the first place (some compilers give a diagnostic about the
special member function being inaccessible even if it's explicitly marked as
deleted; clang is not one such compiler). Do we have a way to rewrite the
access specifier for the special member function as well (kind of like how we
have a way to handle includes we're adding)? I am guessing not yet, but if we
do, that would be fantastic to use here.
Note, I don't think this should hold up your patch or the fixit. A small win is
still a win. :-)
================
Comment at: test/clang-tidy/modernize-use-equals-delete.cpp:93
+private:
+ NegativeConstructNotImpl(const NegativeConstructNotImpl &);
+};
----------------
Phab will not let me delete the unsubmitted comment I had about this, so I am
leaving a junk comment instead. Your move, Phabricator.
https://reviews.llvm.org/D26138
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
