Add a forwarding variadic constructor to llvm::Optional<T>.

[jordan_rose]

Summary

This makes it easier to return values convertible to T when the return type is Optional<T>. This is not a feature in the C++ Library Fundamentals TS for std::experimental::optional, but we've found it useful internally.

This is more controversial than the last three diffs I've posted, because it does make things potentially more ambiguous. In particular, David remarked:

A direct variadic ctor for Optional<T> that builds a T seems a bit subtle/surprising/easily ambiguous - a utility make function is probably more appropriate (make_optional to go with make_unique, etc?).

We've been using this internally already, and the most motivating case was trying to return a type constructed from multiple arguments, i.e.

Optional<std::pair<int, int>> test() {
  return { 1, 2 };
}

There's also an annoyance with implicit conversions needing to be made explicit:

Optional<StringRef> x = ""; // rejected

Optional<StringRef> test() {
  return ""; // rejected
}

But I do know that Implicit Conversions Are Dangerous, and I'd be willing to withdraw this (and go change our internal code), particularly if someone comes up with an example of this going awry.

(I've marked David as a reviewer because he took an interest in my original post, but I'd welcome comments from anyone here.)

[dblaikie]

Any idea if this has been discussed in the C++ standards committee proposal? (be good to know if it was considered & rejected, and if so why)

one thing that might be confusing would be:

Optional<T> func() {
  return {};
}

Does your implementation have trouble with copying a volatile Optional<T>, perhaps? (you've accounted for const/non-const, but not necessarily volatile/non-volatile) I suspect, if we're going for this, the right thing to do is to SFINAE the whole varargs ctor to ensure it's only available when the T is constructible (and, to be precise, /implicitly/ constructible) from the args. This way it won't accidentally steal away Optional(Optional) constructions, etc.

But as with another patch - I'd be concerned that this is just an in-tree trap, since the implementation isn't portable to MSVC, so no in-tree code could realistically use this.

[jordan_rose]

Another interesting case that came up was passing an value of type Foo * to a parameter of type Optional<FooWrapper>.

I did forget to include the implicitly-constructible-only constraint, but I think I'm going to just abandon this; we weren't using it quite as frequently as I remembered, and it's not something the standard will ever do.

include/llvm/ADT/Optional.h

 class Optional {
   AlignedCharArrayUnion<T> storage;
   bool hasVal;
 public:
   typedef T value_type;
 
   Optional(NoneType) : hasVal(false) {}
   explicit Optional() : hasVal(false) {}
+
   Optional(const T &y) : hasVal(true) {
     new (storage.buffer) T(y);
   }
+  // This is necessary for disambiguation purposes.
+  Optional(Optional &O) : hasVal(O.hasVal) {
+    if (hasVal)
+      new (storage.buffer) T(*O);
+  }
   Optional(const Optional &O) : hasVal(O.hasVal) {
     if (hasVal)
       new (storage.buffer) T(*O);
   }
 
   Optional(T &&y) : hasVal(true) {
-    new (storage.buffer) T(std::forward<T>(y));
+    new (storage.buffer) T(std::move(y));
   }
-  Optional(Optional<T> &&O) : hasVal(O) {
+  Optional(Optional &&O) : hasVal(O) {
     if (O) {
       new (storage.buffer) T(std::move(*O));
       O.reset();
     }
   }
-  Optional &operator=(T &&y) {
+  // This is necessary for disambiguation purposes.
+  // It behaves like a copy constructor.
+  Optional(const Optional &&O) : hasVal(O) {
     if (hasVal)
-      **this = std::move(y);
-    else {
-      new (storage.buffer) T(std::move(y));
-      hasVal = true;
-    }
-    return *this;
-  }
-  Optional &operator=(Optional &&O) {
-    if (!O)
-      reset();
-    else {
-      *this = std::move(*O);
-      O.reset();
-    }
-    return *this;
+      new (storage.buffer) T(*O);
   }
 
 #if LLVM_HAS_VARIADIC_TEMPLATES
 
+  /// Construct an instance containing a value of type \c T constructed with
+  /// the given arguments.
+  ///
+  /// \param Args The arguments with which the \c T object will be
+  /// direct-initialized.
+  template<typename ...ArgTypes>
+  /*implicit*/ Optional(ArgTypes &&...Args) : hasVal(true) {
+    new (storage.buffer) T(std::forward<ArgTypes>(Args)...);
+  }
+
   /// Create a new object by constructing it in place with the given arguments.
   template<typename ...ArgTypes>
   void emplace(ArgTypes &&...Args) {
     reset();
     hasVal = true;
     new (storage.buffer) T(std::forward<ArgTypes>(Args)...);
   }
 
+#else // LLVM_HAS_VARIADIC_TEMPLATES
+
+  /// Construct an instance containing a value of type \c T from an argument
+  /// convertible to T, using direct-initialization.
+  template<typename U>
+  /*implicit*/ Optional(U &&Arg) : hasVal(true) {
+    new (storage.buffer) T(std::forward<U>(Arg));
+  }
+
 #endif // LLVM_HAS_VARIADIC_TEMPLATES
 
   static inline Optional create(const T* y) {
     return y ? Optional(*y) : Optional();
   }
 
-  // FIXME: these assignments (& the equivalent const T&/const Optional& ctors)
-  // could be made more efficient by passing by value, possibly unifying them
-  // with the rvalue versions above - but this could place a different set of
-  // requirements (notably: the existence of a default ctor) when implemented
-  // in that way. Careful SFINAE to avoid such pitfalls would be required.
-  Optional &operator=(const T &y) {
-    if (hasVal)
-      **this = y;
-    else {
-      new (storage.buffer) T(y);
+  Optional &operator=(const Optional &O) {
+    if (!O) {
+      reset();
+    } else if (hasVal) {
+      // Prefer T::operator= when possible.
+      **this = *O;
+    } else {
+      new (storage.buffer) T(*O);
       hasVal = true;
     }
     return *this;
   }
-
-  Optional &operator=(const Optional &O) {
-    if (!O)
+  Optional &operator=(Optional &&O) {
+    if (!O) {
       reset();
-    else
-      *this = *O;
+    } else {
+      if (hasVal) {
+        // Prefer T::operator= when possible.
+        **this = *std::move(O);
+      } else {
+        new (storage.buffer) T(*std::move(O));
+        hasVal = true;
+      }
+      O.reset();
+    }
     return *this;
   }
 
   void reset() {
     if (hasVal) {
       (**this).~T();
       hasVal = false;
     }

unittests/ADT/OptionalTest.cpp

NonDefaultConstructible::ResetCounts();
     EXPECT_EQ(0u, NonDefaultConstructible::Destructions);
     NonDefaultConstructible::ResetCounts();
   }
   EXPECT_EQ(0u, NonDefaultConstructible::CopyConstructions);
   EXPECT_EQ(0u, NonDefaultConstructible::CopyAssignments);
   EXPECT_EQ(0u, NonDefaultConstructible::Destructions);
 }
 
+TEST_F(OptionalTest, ConstCopyConstruction) {
+  NonDefaultConstructible::ResetCounts();
+  {
+    const Optional<NonDefaultConstructible> A(NonDefaultConstructible(3));
+    EXPECT_EQ(1u, NonDefaultConstructible::CopyConstructions);
+    EXPECT_EQ(0u, NonDefaultConstructible::CopyAssignments);
+    EXPECT_EQ(1u, NonDefaultConstructible::Destructions);
+    NonDefaultConstructible::ResetCounts();
+    Optional<NonDefaultConstructible> B(A);
+    EXPECT_EQ(1u, NonDefaultConstructible::CopyConstructions);
+    EXPECT_EQ(0u, NonDefaultConstructible::CopyAssignments);
+    EXPECT_EQ(0u, NonDefaultConstructible::Destructions);
+    NonDefaultConstructible::ResetCounts();
+  }
+  EXPECT_EQ(0u, NonDefaultConstructible::CopyConstructions);
+  EXPECT_EQ(0u, NonDefaultConstructible::CopyAssignments);
+  EXPECT_EQ(2u, NonDefaultConstructible::Destructions);
+}
+
+TEST_F(OptionalTest, ConstMoveConstruction) {
+  NonDefaultConstructible::ResetCounts();
+  {
+    const Optional<NonDefaultConstructible> A(NonDefaultConstructible(3));
+    EXPECT_EQ(1u, NonDefaultConstructible::CopyConstructions);
+    EXPECT_EQ(0u, NonDefaultConstructible::CopyAssignments);
+    EXPECT_EQ(1u, NonDefaultConstructible::Destructions);
+    NonDefaultConstructible::ResetCounts();
+    Optional<NonDefaultConstructible> B(std::move(A));
+    EXPECT_EQ(1u, NonDefaultConstructible::CopyConstructions);
+    EXPECT_EQ(0u, NonDefaultConstructible::CopyAssignments);
+    EXPECT_EQ(0u, NonDefaultConstructible::Destructions);
+    NonDefaultConstructible::ResetCounts();
+  }
+  EXPECT_EQ(0u, NonDefaultConstructible::CopyConstructions);
+  EXPECT_EQ(0u, NonDefaultConstructible::CopyAssignments);
+  EXPECT_EQ(2u, NonDefaultConstructible::Destructions);
+}
+
+TEST_F(OptionalTest, ConvertingAssignment) {
+  NonDefaultConstructible::ResetCounts();
+
+  Optional<NonDefaultConstructible> A;
+  A = 5;
+  EXPECT_TRUE(A.hasValue());
+
+  EXPECT_EQ(1u, NonDefaultConstructible::CopyConstructions);
+  EXPECT_EQ(0u, NonDefaultConstructible::CopyAssignments);
+  EXPECT_EQ(1u, NonDefaultConstructible::Destructions);
+  NonDefaultConstructible::ResetCounts();
+
+  A = 10;
+  EXPECT_EQ(0u, NonDefaultConstructible::CopyConstructions);
+  EXPECT_EQ(1u, NonDefaultConstructible::CopyAssignments);
+  EXPECT_EQ(1u, NonDefaultConstructible::Destructions);
+}
+
 TEST_F(OptionalTest, GetValueOr) {
   Optional<int> A;
   EXPECT_EQ(42, A.getValueOr(42));
 
   A = 5;
   EXPECT_EQ(5, A.getValueOr(42));
 }
 
 
 #if LLVM_HAS_VARIADIC_TEMPLATES
 
 struct MultiArgConstructor {
   int x, y;
   MultiArgConstructor(int x, int y) : x(x), y(y) {}
 };
 
+TEST_F(OptionalTest, VariadicConstruction) {
+  Optional<MultiArgConstructor> A(1, 2);
+  EXPECT_TRUE(A.hasValue());
+  EXPECT_EQ(1, A->x);
+  EXPECT_EQ(2, A->y);
+
+  A = { 3, 4 };
+  EXPECT_TRUE(A.hasValue());
+  EXPECT_EQ(3, A->x);
+  EXPECT_EQ(4, A->y);
+}
+
 TEST_F(OptionalTest, Emplace) {
   Optional<MultiArgConstructor> A;
   A.emplace(1, 2);
   EXPECT_TRUE(A.hasValue());
   EXPECT_EQ(1, A->x);
   EXPECT_EQ(2, A->y);
 }
 

 }
 
 TEST_F(OptionalTest, MoveOnlyAssignment) {
   MoveOnly::ResetCounts();
   Optional<MoveOnly> O;
   O = MoveOnly(3);
   EXPECT_TRUE((bool)O);
   EXPECT_EQ(3, O->val);
-  EXPECT_EQ(1u, MoveOnly::MoveConstructions);
+  EXPECT_EQ(2u, MoveOnly::MoveConstructions);
   EXPECT_EQ(0u, MoveOnly::MoveAssignments);
-  EXPECT_EQ(1u, MoveOnly::Destructions);
+  EXPECT_EQ(2u, MoveOnly::Destructions);
 }
 
 TEST_F(OptionalTest, MoveOnlyInitializingAssignment) {
   Optional<MoveOnly> A(MoveOnly(3));
   Optional<MoveOnly> B;
   MoveOnly::ResetCounts();
   B = std::move(A);
   EXPECT_FALSE((bool)A);