Modified ref-qualifiers return type for Optional::value() and Optional::operator*

Summary:
Optional::value() returns a temporary object when the object is an rvalue. This is different in semantics then what boost::optional/std::optional do.

The decision to make the copy or not should be up to the user and not the library. Consider an example:

```
void F(Optional<T> &&opt) {
  T&& t = std::move(opt).get();
  // I know `opt` is alive in this scope, I should be able to keep a rvalue ref to the internals
}
// if we were to return a `T`, that would actually return a new temporary.
```

```
void G(T&& t);
G(std::move(opt).get()); // This could have surprising behavior too !
```

This change modified the return type to be `T&&` and also introduces an extra overload for `const T&&`. Also, deleted two test-cases that assume the lifetime to be extended. This is a breaking change but this brings folly::Optional on parity with other siblings.
Closes https://github.com/facebook/folly/pull/353

Reviewed By: ddrcoder

Differential Revision: D3714962

Pulled By: yfeldblum

fbshipit-source-id: 1794d51590062db4ad02fc8688cb28a06712c076

diff --git a/folly/Optional.h b/folly/Optional.h
index 2b12ccc6a310d5a708cf945403bc0d91a2245c01..e897c38bfb0ed91836be732cabe5d0716162107d 100644 (file)
--- a/folly/Optional.h
+++ b/folly/Optional.h
@@ -206,10 +206,12 @@ class Optional {
     return storage_.value;
   }
 
-  // TODO: This should return Value&& instead of Value. Like with
-  // std::move, the calling code should decide whether it wants the move
-  // to happen or not. See std::optional.
-  Value value() && {
+  Value&& value() && {
+    require_value();
+    return std::move(storage_.value);
+  }
+
+  const Value&& value() const&& {
     require_value();
     return std::move(storage_.value);
   }
@@ -228,12 +230,10 @@ class Optional {
     return hasValue();
   }
 
-  const Value& operator*() const&  { return value(); }
-        Value& operator*()      &  { return value(); }
-        // TODO: This should return Value&& instead of Value. Like with
-        // std::move, the calling code should decide whether it wants the move
-        // to happen or not. See std::optional.
-        Value  operator*()      && { return std::move(value()); }
+  const Value& operator*()  const&  { return value(); }
+        Value& operator*()       &  { return value(); }
+  const Value&& operator*() const&& { return std::move(value()); }
+        Value&& operator*()      && { return std::move(value()); }
 
   const Value* operator->() const { return &value(); }
         Value* operator->()       { return &value(); }

diff --git a/folly/test/OptionalTest.cpp b/folly/test/OptionalTest.cpp
index 1082b35bbd3c72c1ef2663cae1adde999cbe45e9..0fc31e0b95a5d7232b54cdecf119d22392018954 100644 (file)
--- a/folly/test/OptionalTest.cpp
+++ b/folly/test/OptionalTest.cpp
@@ -174,26 +174,12 @@ struct ExpectingDeleter {
   }
 };
 
-TEST(Optional, value_life_extention) {
-  // Extends the life of the value.
-  const auto& ptr = Optional<std::unique_ptr<int, ExpectingDeleter>>(
-      {new int(42), ExpectingDeleter{1337}}).value();
-  *ptr = 1337;
-}
-
 TEST(Optional, value_move) {
   auto ptr = Optional<std::unique_ptr<int, ExpectingDeleter>>(
       {new int(42), ExpectingDeleter{1337}}).value();
   *ptr = 1337;
 }
 
-TEST(Optional, dereference_life_extention) {
-  // Extends the life of the value.
-  const auto& ptr = *Optional<std::unique_ptr<int, ExpectingDeleter>>(
-      {new int(42), ExpectingDeleter{1337}});
-  *ptr = 1337;
-}
-
 TEST(Optional, dereference_move) {
   auto ptr = *Optional<std::unique_ptr<int, ExpectingDeleter>>(
       {new int(42), ExpectingDeleter{1337}});