Permalink
Browse files

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
  • Loading branch information...
1 parent d006f32 commit fd20c97c69af72155fc5d611630b4799eeda6db8 @lovrop lovrop committed with jdelong Mar 11, 2013
Showing with 54 additions and 0 deletions.
  1. +10 −0 folly/Optional.h
  2. +44 −0 folly/test/OptionalTest.cpp
View
@@ -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();
@@ -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());
+ }
+}

0 comments on commit fd20c97

Please sign in to comment.