[PATCH] D18265: [clang-tidy] New: checker misc-assign-operator-return

[Samuel Benzaquen via cfe-commits]

sbenza added inline comments.

================
Comment at: clang-tidy/misc/AssignOperatorCheck.cpp:63
@@ +62,3 @@
+
+  Finder->addMatcher(returnStmt(IsBadReturnStatement, hasAncestor(IsGoodAssign))
+                         .bind("returnStmt"),
----------------
I dislike these uses of hasAnscestor. They are kind of slow.
But more importantly, they break with nested functions/types.
This particular example is not checking that the return statement is from the assignment operator, only that it is within it. For example, it would match a lambda.
I think this would trip the check:

    F& operator=(const F& o) {
      std::copy_if(o.begin(), o.end(), begin(), [](V v) { return v > 0; });
      return *this;
    }

================
Comment at: clang-tidy/misc/AssignOperatorCheck.cpp:69
@@ +68,3 @@
+void AssignOperatorCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *Method = Result.Nodes.getNodeAs<CXXMethodDecl>("method");
+  const auto *RetStmt = Result.Nodes.getNodeAs<ReturnStmt>("returnStmt");
----------------
Move this closer to where it is used.

================
Comment at: clang-tidy/misc/AssignOperatorCheck.cpp:70
@@ +69,3 @@
+  const auto *Method = Result.Nodes.getNodeAs<CXXMethodDecl>("method");
+  const auto *RetStmt = Result.Nodes.getNodeAs<ReturnStmt>("returnStmt");
+  if (RetStmt) {
----------------
Move this into the if () statement.

================
Comment at: clang-tidy/misc/AssignOperatorCheck.h:19
@@ +18,3 @@
+
+/// Finds declarations of assignment operators with the wrong return and/or
+/// argument types.
----------------
This does not talk about the return statement, only the return type.

================
Comment at: test/clang-tidy/misc-assign-operator.cpp:16
@@ +15,3 @@
+  AlsoGood& operator=(AlsoGood);
+};
+
----------------
This is a very common C++98 way of implementing copy-and-swap with copy elision support.
You do: `T& operator=(T t) { swap(t); return *this; }`
And it will avoid the copy if the argument is already a temporary due to copy elision on the caller.

================
Comment at: test/clang-tidy/misc-assign-operator.cpp:69
@@ +68,3 @@
+    n = rhs.n;
+    return *this;
+  }
----------------
This case is not a bad return statement, so it should not be in this class.


http://reviews.llvm.org/D18265