Commit ca5621fa authored by Philip Pronin's avatar Philip Pronin Committed by Sara Golemon

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
parent b1463f8f
...@@ -190,8 +190,8 @@ struct Synchronized { ...@@ -190,8 +190,8 @@ struct Synchronized {
* constructor. * constructor.
*/ */
Synchronized() = default; Synchronized() = default;
/**
/**
* Copy constructor copies the data (with locking the source and * Copy constructor copies the data (with locking the source and
* all) but does NOT copy the mutex. Doing so would result in * all) but does NOT copy the mutex. Doing so would result in
* deadlocks. * deadlocks.
...@@ -220,7 +220,7 @@ struct Synchronized { ...@@ -220,7 +220,7 @@ struct Synchronized {
* Constructor taking a datum rvalue as argument moves it. Again, * Constructor taking a datum rvalue as argument moves it. Again,
* there is no need to lock the constructing object. * 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 * The canonical assignment operator only assigns the data, NOT the
...@@ -228,7 +228,9 @@ struct Synchronized { ...@@ -228,7 +228,9 @@ struct Synchronized {
* addresses. * addresses.
*/ */
Synchronized& operator=(const Synchronized& rhs) { Synchronized& operator=(const Synchronized& rhs) {
if (this < *rhs) { if (this == &rhs) {
// Self-assignment, pass.
} else if (this < &rhs) {
auto guard1 = operator->(); auto guard1 = operator->();
auto guard2 = rhs.operator->(); auto guard2 = rhs.operator->();
datum_ = rhs.datum_; datum_ = rhs.datum_;
...@@ -240,6 +242,26 @@ struct Synchronized { ...@@ -240,6 +242,26 @@ struct Synchronized {
return *this; 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. * Lock object, assign datum.
*/ */
...@@ -249,6 +271,15 @@ struct Synchronized { ...@@ -249,6 +271,15 @@ struct Synchronized {
return *this; 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) * A LockedPtr lp keeps a modifiable (i.e. non-const)
* Synchronized<T> object locked for the duration of lp's * Synchronized<T> object locked for the duration of lp's
...@@ -519,7 +550,9 @@ struct Synchronized { ...@@ -519,7 +550,9 @@ struct Synchronized {
} }
auto guard1 = operator->(); auto guard1 = operator->();
auto guard2 = rhs.operator->(); auto guard2 = rhs.operator->();
datum_.swap(rhs.datum_);
using std::swap;
swap(datum_, rhs.datum_);
} }
/** /**
...@@ -528,7 +561,9 @@ struct Synchronized { ...@@ -528,7 +561,9 @@ struct Synchronized {
*/ */
void swap(T& rhs) { void swap(T& rhs) {
LockedPtr guard = operator->(); LockedPtr guard = operator->();
datum_.swap(rhs);
using std::swap;
swap(datum_, rhs);
} }
/** /**
......
...@@ -183,6 +183,9 @@ An additional assignment operator accepts a `const T&` on the ...@@ -183,6 +183,9 @@ An additional assignment operator accepts a `const T&` on the
right-hand side. The operator copies the datum inside a right-hand side. The operator copies the datum inside a
critical section. critical section.
In addition to assignment operators, `Synchronized<T>` has move
assignment operators.
An additional `swap` method accepts a `T&` and swaps the data An additional `swap` method accepts a `T&` and swaps the data
inside a critical section. This is by far the preferred method of inside a critical section. This is by far the preferred method of
changing the guarded datum wholesale because it keeps the lock changing the guarded datum wholesale because it keeps the lock
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment