Add explicit assignment operator definitions to Optional

Summary:
See new test.  Under GCC 4.6 (which is what the folly tests build with) the old code works fine but under 4.7 building fails with:

folly/test/OptionalTest.cpp: In member function ‘virtual void Optional_AssignmentContained_Test::TestBody()’:
folly/test/OptionalTest.cpp:122:14: error: use of deleted function ‘ContainsOptional& ContainsOptional::operator=(const ContainsOptional&)’
folly/test/OptionalTest.cpp:106:21: note: ‘ContainsOptional& ContainsOptional::operator=(const ContainsOptional&)’ is implicitly deleted because the default definition would be ill-formed:
folly/test/OptionalTest.cpp:106:21: error: use of deleted function ‘folly::Optional<int>& folly::Optional<int>::operator=(const folly::Optional<int>&)’
In file included from folly/test/OptionalTest.cpp:17:0:
./folly/Optional.h:84:7: note: ‘folly::Optional<int>& folly::Optional<int>::operator=(const folly::Optional<int>&)’ is implicitly declared as deleted because ‘folly::Optional<int>’ declares a move constructor or move assignment operator
folly/test/OptionalTest.cpp:129:30: error: use of deleted function ‘ContainsOptional& ContainsOptional::operator=(ContainsOptional&&)’
folly/test/OptionalTest.cpp:108:21: note: ‘ContainsOptional& ContainsOptional::operator=(ContainsOptional&&)’ is implicitly deleted because the default definition would be ill-formed:
folly/test/OptionalTest.cpp:108:21: error: non-static data member ‘ContainsOptional::opt_’ does not have a move assignment operator or trivial copy assignment operator
folly/test/OptionalTest.cpp:137:14: error: use of deleted function ‘ContainsOptional& ContainsOptional::operator=(const ContainsOptional&)’

Test Plan:
(1) fbconfig folly/test && fbmake dbg && _build/dbg/folly/test/optional_test
(2) Remove folly/PLATFORM to build with gcc 4.7, then repeat (1).  Without the code fix, the new test fails to build.  With the fix, the test builds and runs fine.

Reviewed By: tjackson@fb.com

FB internal diff: D732402

diff --git a/folly/Optional.h b/folly/Optional.h
index 82070889e32d17a812af444b8b474532bb626e6c..871357ceabb3168b29b3da49c13886482d1d11dd 100644 (file)
--- a/folly/Optional.h
+++ b/folly/Optional.h
@@ -169,6 +169,16 @@ class Optional : boost::totally_ordered<Optional<Value>,
     return *this;
   }
 
+  Optional& operator=(Optional &&other) {
+    assign(std::move(other));
+    return *this;
+  }
+
+  Optional& operator=(const Optional &other) {
+    assign(other);
+    return *this;
+  }
+
   bool operator<(const Optional& other) const {
     if (hasValue() != other.hasValue()) {
       return hasValue() < other.hasValue();

diff --git a/folly/test/OptionalTest.cpp b/folly/test/OptionalTest.cpp
index 4eb564176fc988cca7fc0461bd058f5ee2f84f25..68e81649032a71d4a8797a364da13af01c56218c 100644 (file)
--- a/folly/test/OptionalTest.cpp
+++ b/folly/test/OptionalTest.cpp
@@ -309,3 +309,47 @@ TEST(Optional, MakeOptional) {
   ASSERT_TRUE(optIntPtr.hasValue());
   EXPECT_EQ(**optIntPtr, 3);
 }
+
+class ContainsOptional {
+ public:
+  ContainsOptional() { }
+  explicit ContainsOptional(int x) : opt_(x) { }
+  bool hasValue() const { return opt_.hasValue(); }
+  int value() const { return opt_.value(); }
+
+  ContainsOptional(const ContainsOptional &other) = default;
+  ContainsOptional& operator=(const ContainsOptional &other) = default;
+  ContainsOptional(ContainsOptional &&other) = default;
+  ContainsOptional& operator=(ContainsOptional &&other) = default;
+
+ private:
+  Optional<int> opt_;
+};
+
+/**
+ * Test that a class containing an Optional can be copy and move assigned.
+ * This was broken under gcc 4.7 until assignment operators were explicitly
+ * defined.
+ */
+TEST(Optional, AssignmentContained) {
+  {
+    ContainsOptional source(5), target;
+    target = source;
+    EXPECT_TRUE(target.hasValue());
+    EXPECT_EQ(5, target.value());
+  }
+
+  {
+    ContainsOptional source(5), target;
+    target = std::move(source);
+    EXPECT_TRUE(target.hasValue());
+    EXPECT_EQ(5, target.value());
+    EXPECT_FALSE(source.hasValue());
+  }
+
+  {
+    ContainsOptional opt_uninit, target(10);
+    target = opt_uninit;
+    EXPECT_FALSE(target.hasValue());
+  }
+}