[PATCH] D16376: clang-tidy check: rule-of-five

[Alexander Kornienko via cfe-commits]

alexfh added inline comments.

================
Comment at: clang-tidy/misc/RuleOfFiveCheck.cpp:30
@@ +29,3 @@
+                                            .bind("copy-ctor")),
+          unless(hasDescendant(cxxMethodDecl(isCopyAssignmentOperator())))),
+      this);
----------------
Why is `hasDescendant` needed? Doesn't just `has(...)` work? I think, methods are direct children of the CXXRecordDecl they are declared in.

Same applies to other matchers. 

================
Comment at: clang-tidy/misc/RuleOfFiveCheck.cpp:80
@@ +79,3 @@
+  case RuleOfFiveCheck::SpecialFunctionKind::MoveAssignment: {
+    SourceLocation DeclBegin = MatchedDecl.getLocStart();
+    return DeclBegin.getLocWithOffset(-1);
----------------
The variable doesn't make the code shorter or easier to read. Please remove it.

================
Comment at: clang-tidy/misc/RuleOfFiveCheck.cpp:110
@@ +109,3 @@
+    return (llvm::Twine("\n") + ClassName + " &operator=(const " + ClassName +
+            " &) = " + deleteOrDefault(MatchedDecl) + ";")
+        .str();
----------------
How about removing the `deleteOrDefault` function and inserting this before the switch:

  StringRef deleteOrDefault = MethodDecl.isDefaulted() ? " = default;" : " = delete;";

?

================
Comment at: clang-tidy/misc/RuleOfFiveCheck.cpp:117
@@ +116,3 @@
+  case RuleOfFiveCheck::SpecialFunctionKind::MoveConstructor:
+    return (llvm::Twine("\n") + ClassName + " &operator=(" + ClassName +
+            " &&) = " + deleteOrDefault(MatchedDecl) + ";")
----------------
We usually rely on clang-format to clean up formatting after applying fixes, but in some cases clang-format looks into possible preferences already present in the code. Placement of `*` or `&` on pointers and references is one of such things. I think, clang-tidy should keep neutrality in this regard, e.g. by adding spaces on both sides of the `*` an `&` in its fixes.

================
Comment at: test/clang-tidy/misc-rule-of-five.cpp:1
@@ +1,2 @@
+// RUN: %check_clang_tidy %s misc-rule-of-five %t
+
----------------
Please add tests with macros and templates with multiple instantiations.


http://reviews.llvm.org/D16376