Overload Optional::value() on object reference type.

Summary: `folly::Optional` returns the stored value by reference when the object
is an rvalue.  This causes three issues:

  * If the user calls `value()` on an rvalue `Optional`, and assigns the result
    to a new variable, the copy constructor gets called, instead of the move
    constructor.  This causes the added test `value_move` to not compile.
  * If the user calls `value()` on an rvalue `Optional`, and assigns the result
    to a const lvalue reference, they might expect the lifetime to be extended
    when it isn't. See the added test `value_life_extention`.
  * Assigning the results of `value()` on an rvalue `Optional` to a mutable
    lvalue reference compiled in the old code, when it shouldn't, because that
    is always a dangling reference as far as I can tell.

I'm not sure how strict `folly` is with compatibility, but I believe the
breakage would be minimal, and any code that gets broken probably deserves it.

I'm not exactly sure who I should add as a reviewer, so hopefully herald has got
my back.

Reviewed By: @yfeldblum

Differential Revision: D2249548

diff --git a/folly/Optional.h b/folly/Optional.h
index 9947fd4adf05a864ef149e80da30e4f5982946e5..def3670f00ab69946c0fce6eb12441eebd4466dd 100644 (file)
--- a/folly/Optional.h
+++ b/folly/Optional.h
@@ -211,18 +211,24 @@ class Optional {
     }
   }
 
-  const Value& value() const {
+  const Value& value() const& {
     require_value();
     return value_;
   }
 
-  Value& value() {
+  Value& value() & {
     require_value();
     return value_;
   }
 
-  Value* get_pointer() { return hasValue_ ? &value_ : nullptr; }
-  const Value* get_pointer() const { return hasValue_ ? &value_ : nullptr; }
+  Value value() && {
+    require_value();
+    return std::move(value_);
+  }
+
+  const Value* get_pointer() const&  { return hasValue_ ? &value_ : nullptr; }
+        Value* get_pointer()      &  { return hasValue_ ? &value_ : nullptr; }
+        Value* get_pointer()      && = delete;
 
   bool hasValue() const { return hasValue_; }
 
@@ -230,8 +236,9 @@ class Optional {
     return hasValue();
   }
 
-  const Value& operator*() const { return value(); }
-        Value& operator*()       { return value(); }
+  const Value& operator*() const&  { return value(); }
+        Value& operator*()      &  { return 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 72c2734591670aa2bb0ec7e0c6be987bd7c0e9c5..974c8f422f6fa03cdc1c981919e4e440473df254 100644 (file)
--- a/folly/test/OptionalTest.cpp
+++ b/folly/test/OptionalTest.cpp
@@ -159,6 +159,41 @@ TEST(Optional, value_or_noncopyable) {
   EXPECT_EQ(42, *std::move(opt).value_or(std::move(dflt)));
 }
 
+struct ExpectingDeleter {
+  explicit ExpectingDeleter(int expected) : expected(expected) { }
+  int expected;
+  void operator()(const int* ptr) {
+    EXPECT_EQ(*ptr, expected);
+    delete ptr;
+  }
+};
+
+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}});
+  *ptr = 1337;
+}
+
 TEST(Optional, EmptyConstruct) {
   Optional<int> opt;
   EXPECT_FALSE(bool(opt));