move assignment operators for folly::Synchronized

Summary:
* added move assignment operators,
* fixed `operator=(const Synchronized& rhs)` (it had a typo),
* fixed deadlock on self-assignment,
* changed `swap` to call `swap(lhs.datum_, rhs.datum_)` instead of `lhs.datum_.swap(rhs.datum_)`.

Test Plan: fbconfig -r folly/test && fbmake opt -j32 && fbmake runtests_opt

Reviewed By: delong.j@fb.com

FB internal diff: D875977

diff --git a/folly/Synchronized.h b/folly/Synchronized.h
index cc43071aa865deeb0ffec1b75fe95621d572c59f..d632a772c5e8ba140f2341d78298eaf06c57ce5b 100644 (file)
--- a/folly/Synchronized.h
+++ b/folly/Synchronized.h
@@ -190,8 +190,8 @@ struct Synchronized {
    * constructor.
    */
   Synchronized() = default;
-  /**
 
+  /**
    * Copy constructor copies the data (with locking the source and
    * all) but does NOT copy the mutex. Doing so would result in
    * deadlocks.
@@ -220,7 +220,7 @@ struct Synchronized {
    * Constructor taking a datum rvalue as argument moves it. Again,
    * there is no need to lock the constructing object.
    */
-  explicit Synchronized(T && rhs) : datum_(std::move(rhs)) {}
+  explicit Synchronized(T&& rhs) : datum_(std::move(rhs)) {}
 
   /**
    * The canonical assignment operator only assigns the data, NOT the
@@ -228,7 +228,9 @@ struct Synchronized {
    * addresses.
    */
   Synchronized& operator=(const Synchronized& rhs) {
-    if (this < *rhs) {
+    if (this == &rhs) {
+      // Self-assignment, pass.
+    } else if (this < &rhs) {
       auto guard1 = operator->();
       auto guard2 = rhs.operator->();
       datum_ = rhs.datum_;
@@ -240,6 +242,26 @@ struct Synchronized {
     return *this;
   }
 
+  /**
+   * Move assignment operator, only assigns the data, NOT the
+   * mutex. It locks the two objects in ascending order of their
+   * addresses.
+   */
+  Synchronized& operator=(Synchronized&& rhs) {
+    if (this == &rhs) {
+      // Self-assignment, pass.
+    } else if (this < &rhs) {
+      auto guard1 = operator->();
+      auto guard2 = rhs.operator->();
+      datum_ = std::move(rhs.datum_);
+    } else {
+      auto guard1 = rhs.operator->();
+      auto guard2 = operator->();
+      datum_ = std::move(rhs.datum_);
+    }
+    return *this;
+  }
+
   /**
    * Lock object, assign datum.
    */
@@ -249,6 +271,15 @@ struct Synchronized {
     return *this;
   }
 
+  /**
+   * Lock object, move-assign datum.
+   */
+  Synchronized& operator=(T&& rhs) {
+    auto guard = operator->();
+    datum_ = std::move(rhs);
+    return *this;
+  }
+
   /**
    * A LockedPtr lp keeps a modifiable (i.e. non-const)
    * Synchronized<T> object locked for the duration of lp's
@@ -519,7 +550,9 @@ struct Synchronized {
     }
     auto guard1 = operator->();
     auto guard2 = rhs.operator->();
-    datum_.swap(rhs.datum_);
+
+    using std::swap;
+    swap(datum_, rhs.datum_);
   }
 
   /**
@@ -528,7 +561,9 @@ struct Synchronized {
    */
   void swap(T& rhs) {
     LockedPtr guard = operator->();
-    datum_.swap(rhs);
+
+    using std::swap;
+    swap(datum_, rhs);
   }
 
   /**

diff --git a/folly/docs/Synchronized.md b/folly/docs/Synchronized.md
index a7e2768074ba45c7b4cf9c04c673fad848b976b4..e36f1dbbdfbdaf7f472cd2a8c0b0ec78e88423f1 100644 (file)
--- a/folly/docs/Synchronized.md
+++ b/folly/docs/Synchronized.md
@@ -183,6 +183,9 @@ An additional assignment operator accepts a `const T&` on the
 right-hand side. The operator copies the datum inside a
 critical section.
 
+In addition to assignment operators, `Synchronized<T>` has move
+assignment operators.
+
 An additional `swap` method accepts a `T&` and swaps the data
 inside a critical section. This is by far the preferred method of
 changing the guarded datum wholesale because it keeps the lock