Synchronized: disable operator= when the type isn't copy assignable

Summary: The value of std::is_copy_assignable<folly::Synchronized<T>>::value is incorrect when T isn't copy assignable. As a result, it isn't possible to use SFINAE to properly select a function when the base type is a folly::Synchronized. This diff selectively deletes the copy constructor and copy assignment operator when the underlying type T isn't copyable. This is most useful in the case of folly::Synchronized<std::unique_ptr<...>>

Reviewed By: yfeldblum

Differential Revision: D4203081

fbshipit-source-id: 1e811f9e52db26c23b1c6f1907bac9e2854ddb9d

diff --git a/folly/Synchronized.h b/folly/Synchronized.h
index afbaef2e0f38ad9675402e36e81ac4a436c74040..aeae0549c96643d24e97b96e5797777894ea3f96 100644 (file)
--- a/folly/Synchronized.h
+++ b/folly/Synchronized.h
@@ -433,6 +433,9 @@ struct Synchronized : public SynchronizedBase<
   static constexpr bool nxMoveCtor{
       std::is_nothrow_move_constructible<T>::value};
 
+  // used to disable copy construction and assignment
+  class NonImplementedType;
+
  public:
   using LockedPtr = typename Base::LockedPtr;
   using ConstLockedPtr = typename Base::ConstLockedPtr;
@@ -453,7 +456,11 @@ struct Synchronized : public SynchronizedBase<
    * Note that the copy constructor may throw because it acquires a lock in
    * the contextualRLock() method
    */
-  Synchronized(const Synchronized& rhs) /* may throw */
+ public:
+  /* implicit */ Synchronized(typename std::conditional<
+                              std::is_copy_constructible<T>::value,
+                              const Synchronized&,
+                              NonImplementedType>::type rhs) /* may throw */
       : Synchronized(rhs, rhs.contextualRLock()) {}
 
   /**
@@ -495,7 +502,10 @@ struct Synchronized : public SynchronizedBase<
    * mutex. It locks the two objects in ascending order of their
    * addresses.
    */
-  Synchronized& operator=(const Synchronized& rhs) {
+  Synchronized& operator=(typename std::conditional<
+                          std::is_copy_assignable<T>::value,
+                          const Synchronized&,
+                          NonImplementedType>::type rhs) {
     if (this == &rhs) {
       // Self-assignment, pass.
     } else if (this < &rhs) {

diff --git a/folly/test/SynchronizedTest.cpp b/folly/test/SynchronizedTest.cpp
index adbac4194a735f987371cea1e83ed7a072d67edc..fb17f920c333b97d0d058d62c888fe7514eec691 100644 (file)
--- a/folly/test/SynchronizedTest.cpp
+++ b/folly/test/SynchronizedTest.cpp
@@ -395,6 +395,22 @@ TEST_F(SynchronizedLockTest, NestedSyncUnSync2) {
   EXPECT_EQ((CountPair{4, 4}), FakeMutex::getLockUnlockCount());
 }
 
+TEST_F(SynchronizedLockTest, TestCopyConstructibleValues) {
+  struct NonCopyConstructible {
+    NonCopyConstructible(const NonCopyConstructible&) = delete;
+    NonCopyConstructible& operator=(const NonCopyConstructible&) = delete;
+  };
+  struct CopyConstructible {};
+  EXPECT_FALSE(std::is_copy_constructible<
+               folly::Synchronized<NonCopyConstructible>>::value);
+  EXPECT_FALSE(std::is_copy_assignable<
+               folly::Synchronized<NonCopyConstructible>>::value);
+  EXPECT_TRUE(std::is_copy_constructible<
+              folly::Synchronized<CopyConstructible>>::value);
+  EXPECT_TRUE(
+      std::is_copy_assignable<folly::Synchronized<CopyConstructible>>::value);
+}
+
 TEST_F(SynchronizedLockTest, UpgradableLocking) {
   folly::Synchronized<int, FakeAllPowerfulAssertingMutex> sync;