aaron.ballman added inline comments.
================
Comment at: clang-tidy/cppcoreguidelines/RuleOfFiveAndZeroCheck.cpp:63
@@ +62,3 @@
+ if (const auto *MatchedDecl = Result.Nodes.getNodeAs<CXXRecordDecl>(Match)) {
+
+ diag(MatchedDecl->getLocation(), "class %0 defines a %1 but does not "
----------------
No newline.
================
Comment at: clang-tidy/cppcoreguidelines/RuleOfFiveAndZeroCheck.cpp:72
@@ +71,3 @@
+void RuleOfFiveAndZeroCheck::check(const MatchFinder::MatchResult &Result) {
+
+ checkRuleOfFiveViolation(Result, "dtor", "destructor");
----------------
No newline.
================
Comment at: clang-tidy/cppcoreguidelines/RuleOfFiveAndZeroCheck.cpp:73-77
@@ +72,7 @@
+
+ checkRuleOfFiveViolation(Result, "dtor", "destructor");
+ checkRuleOfFiveViolation(Result, "copy-ctor", "copy constructor");
+ checkRuleOfFiveViolation(Result, "copy-assign", "copy assignment operator");
+ checkRuleOfFiveViolation(Result, "move-ctor", "move constructor");
+ checkRuleOfFiveViolation(Result, "move-assign", "move assignment operator");
+}
----------------
Prazek wrote:
> I think it would be much better to aggregate the diagnosics.
> E.g. if I declare 4 special functions then I will get 4 lines of warnings,
> and I won't even know what function did I forgot to declare.
>
> So it should be better to fire diag on the first, or just one of the special
> function and say "class %0 defines {{list_here}} but doesn't define
> {{other_list}}"
I tend to agree -- this will get very chatty if I define 4 out of the 5 special
member functions, so it might be best if the diagnostic instead reads:
"class %0 defines %1, but does not define %2"
Where %1 is the list of things the class defines and %2 is the list of things
the class fails to define.
Note, "define or delete" is a bit weird because a deleted function is a
definition, just like an explicitly-defaulted function is a definition. It
seems strange to mention delete but not default, so I just went with "define"
by itself for brevity.
================
Comment at: clang-tidy/cppcoreguidelines/RuleOfFiveAndZeroCheck.h:19
@@ +18,3 @@
+
+/// Checks for classes where some, but not all, of the special member functions
+/// are defined.
----------------
Prazek wrote:
> no comma after all. But I might be wrong because I am not certified grammar
> nazi
The comma is correct there. :-)
================
Comment at: test/clang-tidy/cppcoreguidelines-rule-of-five-and-zero.cpp:44
@@ +43,2 @@
+ ~DeletesEverything();
+};
----------------
I'd also like to see a test case with mixed =default and =delete (to ensure we
do not trigger when explicitly defaulting or deleting the some special member
functions).
Repository:
rL LLVM
https://reviews.llvm.org/D22513
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
