[PATCH] D22513: [clang-tidy] add check cppcoreguidelines-rule-of-five-and-zero

[Aaron Ballman via cfe-commits]

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