From: Eric Niebler <eniebler@fb.com>
Date: Thu, 30 Mar 2017 21:09:34 +0000 (-0700)
Subject: Remove unneccessary test for &that==this in folly::Function's move assignment operator.
X-Git-Tag: v2017.04.03.00~5
X-Git-Url: http://plrg.eecs.uci.edu/git/?p=folly.git;a=commitdiff_plain;h=382b70adaa6fbce5ab568fc23a825483ed9df93f

Remove unneccessary test for &that==this in folly::Function's move assignment operator.

Summary: Self-moves are exceedingly rare and need not preserve the state of the object. They must only leave the object in a valid but unspecified state. By removing the branch, we make the common case (non-self move) faster.

Reviewed By: spacedentist, ot

Differential Revision: D4803486

fbshipit-source-id: 3ef2e1e13cd08d9221ecb154bfb3338b16487717
---

diff --git a/folly/Function.h b/folly/Function.h
index 6c64647d..a55381fb 100644
--- a/folly/Function.h
+++ b/folly/Function.h
@@ -578,17 +578,23 @@ class Function final : private detail::function::FunctionTraits<FunctionType> {
 
   /**
    * Move assignment operator
+   *
+   * \note Leaves `that` in a valid but unspecified state. If `&that == this`
+   * then `*this` is left in a valid but unspecified state.
    */
   Function& operator=(Function&& that) noexcept {
-    if (&that != this) {
-      // Q: Why is is safe to destroy and reconstruct this object in place?
-      // A: Two reasons: First, `Function` is a final class, so in doing this
-      //    we aren't slicing off any derived parts. And second, the move
-      //    operation is guaranteed not to throw so we always leave the object
-      //    in a valid state.
-      this->~Function();
-      ::new (this) Function(std::move(that));
-    }
+    // Q: Why is is safe to destroy and reconstruct this object in place?
+    // A: Two reasons: First, `Function` is a final class, so in doing this
+    //    we aren't slicing off any derived parts. And second, the move
+    //    operation is guaranteed not to throw so we always leave the object
+    //    in a valid state.
+    // In the case of self-move (this == &that), this leaves the object in
+    // a default-constructed state. First the object is destroyed, then we
+    // pass the destroyed object to the move constructor. The first thing the
+    // move constructor does is default-construct the object. That object is
+    // "moved" into itself, which is a no-op for a default-constructed Function.
+    this->~Function();
+    ::new (this) Function(std::move(that));
     return *this;
   }
 
diff --git a/folly/test/FunctionTest.cpp b/folly/test/FunctionTest.cpp
index 3fda7cbe..546fb6f4 100644
--- a/folly/test/FunctionTest.cpp
+++ b/folly/test/FunctionTest.cpp
@@ -1045,11 +1045,27 @@ TEST(Function, EmptyAfterConstCast) {
   EXPECT_FALSE(func2);
 }
 
-TEST(Function, SelfMoveAssign) {
-  Function<int()> f = [] { return 0; };
+TEST(Function, SelfStdSwap) {
+  Function<int()> f = [] { return 42; };
+  f.swap(f);
+  EXPECT_TRUE(bool(f));
+  EXPECT_EQ(42, f());
+  std::swap(f, f);
+  EXPECT_TRUE(bool(f));
+  EXPECT_EQ(42, f());
+  folly::swap(f, f);
+  EXPECT_TRUE(bool(f));
+  EXPECT_EQ(42, f());
+}
+
+TEST(Function, SelfMove) {
+  Function<int()> f = [] { return 42; };
   Function<int()>& g = f;
-  f = std::move(g);
+  f = std::move(g); // shouldn't crash!
+  (void)bool(f); // valid but unspecified state
+  f = [] { return 43; };
   EXPECT_TRUE(bool(f));
+  EXPECT_EQ(43, f());
 }
 
 TEST(Function, DeducableArguments) {