Add an emplace(...) method to llvm::Optional<T>.

[jordan_rose]

Summary

This can be used for in-place initialization of non-moveable types. For compilers that don't support variadic templates, only up to four arguments are supported. We can always add more, of course, but this should be good enough until we move to a later MSVC that has full support for variadic templates.e

Inspired by std::experimental::optional from the "Library Fundamentals" C++ TS (API, spec).

[jordan_rose]

I've omitted the std::experimental::optional overload that takes a std::initializer_list and additional arguments. We haven't yet found a use for it, and we can always add it later.

[dblaikie]

include/llvm/ADT/Optional.h

77

This is a tricky thing to upstream without a fallback. Do we have any other cases where we've added a variadic template without a fallback for non-variadic-template-supporting compilers? Without precedent I'd be a little hesitant to be the first example of that situation. Essentially no LLVM code would realistically use this API without a fallback, I'd imagine.

[jordan_rose]

We're using it internally in our own version of Optional; my guess would be that it would only be for projects that had stricter requirements than LLVM and Clang do (like lld, which requires MSVS 2012). However, I could add the fake-variadic up-to-six-arguments fallback for now.

[jordan_rose]

Oops, I meant "like lld used to". The requirements are now in sync with the rest of LLVM.

[dblaikie]

In D5508#6, @jordan_rose wrote:

We're using it internally in our own version of Optional; my guess would be that it would only be for projects that had stricter requirements than LLVM and Clang do (like lld, which requires MSVS 2012). However, I could add the fake-variadic up-to-six-arguments fallback for now.

I think it's probably reasonable for you to either keep it internally (not like it'll conflict with anything) or add the fake-variadic in line with other variadics in the LLVM codebase, so we don't have code that's just have a trap for the unwary.

(we also have some variadic helper utility class too - not sure what that one was for and whether it helps here - might be worth looking at (include/llvm/ADT/VariadicFunction.h)

[jordan_rose]

Added non-variadic overloads for 0-4 arguments. What do you think?

[dblaikie]

A few remaining questions, but I assume they're mostly in the negative (and/or the test case improvements seem obvious enough to not require another round of review)

include/llvm/ADT/Optional.h

86

I haven't looked - but I take it we have no better mechanism for stamping out these overloads more economically?

Do we have a fairly consistent habit on the number of args supported in other faux variadic situations?

unittests/ADT/OptionalTest.cpp

182

Maybe make this class non-copyable and non-movable, to ensure that emplace can be used on a non-movable type?

(or add that as a separate test - with a clear "ImmovableType" or something for testing (some of these basic feature types might eventually get pulled out into common test utility headers for use in a variety of container tests))

182

Want to try adding an explicit ctor (or making this ctor explicit) just to demonstrate that emplace can/should be able to call explicit ctors? (I assume that's the intent - I haven't checked the standard proposal, etc)

[jordan_rose]

include/llvm/ADT/Optional.h

86

I checked VariadicFunction.h but that only works when the type is consistent. I could try to macro it up but it doesn't seem like that much of a win, especially when the code will probably be gone in two years.

The only other use of LLVM_HAS_VARIADIC_TEMPLATES I see is our implementation of function_ref (a class I didn't know about). That one only goes up to three arguments. I'll admit I didn't use any particular logic in picking four other than "this should hold us for a while"; the cases where we use it only go up to two.

unittests/ADT/OptionalTest.cpp

182

Good point, will do.

182

The MoveOnlyEmplace test below does test that—although it's not a non-movable type, it does track how many times the move constructor is called, and that's 0.

[dblaikie]

unittests/ADT/OptionalTest.cpp

182

Yeah - in my experience it's pretty easy to introduce a compile time dependency on the existence of certain operations even if those operations are never actually executed.

(eg: clear() could be implemented as erase(begin, end) but that requires elements to be movable to move them down to fill the space (even though, dynamically, there's nothing to move down), unique_ptr(nullptr) implemented in terms of unique_ptr() (forwarding ctor) ends up meaning that unique_ptr(nullptr) needs access to the underlying object's dtor... , etc)

[jordan_rose]

Committed in r218732, with test changes applied. I didn't end up making a general ImmovableType (ImmobileType?) because the test would have looked exactly the same.

include/llvm/ADT/Optional.h

 #include "llvm/Support/AlignOf.h"
 #include "llvm/Support/Compiler.h"
 #include <cassert>
+#include <new>
 #include <utility>
 
 namespace llvm {

     return *this;
   }
 
+#if LLVM_HAS_VARIADIC_TEMPLATES
+
+  /// 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
+  
+  /// Create a new object by default-constructing it in place.
+  void emplace() {
+    reset();
+    hasVal = true;
+    new (storage.buffer) T();
+  }
+  
+  /// Create a new object by constructing it in place with the given arguments.
+  template<typename T1>
+  void emplace(T1 &&A1) {
+    reset();
+    hasVal = true;
+    new (storage.buffer) T(std::forward<T1>(A1));
+  }
+  
+  /// Create a new object by constructing it in place with the given arguments.
+  template<typename T1, typename T2>
+  void emplace(T1 &&A1, T2 &&A2) {
+    reset();
+    hasVal = true;
+    new (storage.buffer) T(std::forward<T1>(A1), std::forward<T2>(A2));
+  }
+  
+  /// Create a new object by constructing it in place with the given arguments.
+  template<typename T1, typename T2, typename T3>
+  void emplace(T1 &&A1, T2 &&A2, T3 &&A3) {
+    reset();
+    hasVal = true;
+    new (storage.buffer) T(std::forward<T1>(A1), std::forward<T2>(A2),
+        std::forward<T3>(A3));
+  }
+  
+  /// Create a new object by constructing it in place with the given arguments.
+  template<typename T1, typename T2, typename T3, typename T4>
+  void emplace(T1 &&A1, T2 &&A2, T3 &&A3, T4 &&A4) {
+    reset();
+    hasVal = true;
+    new (storage.buffer) T(std::forward<T1>(A1), std::forward<T2>(A2),
+        std::forward<T3>(A3), std::forward<T4>(A4));
+  }
+
+#endif // LLVM_HAS_VARIADIC_TEMPLATES
+
   static inline Optional create(const T* y) {
     return y ? Optional(*y) : Optional();
   }

unittests/ADT/OptionalTest.cpp

   EXPECT_EQ(5, A.getValueOr(42));
 }
 
+struct MultiArgConstructor {
+  int x, y;
+  MultiArgConstructor(int x, int y) : x(x), y(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);
+}
+
 struct MoveOnly {
   static unsigned MoveConstructions;
   static unsigned Destructions;

   EXPECT_EQ(1u, MoveOnly::Destructions);
 }
 
+TEST_F(OptionalTest, MoveOnlyEmplace) {
+  Optional<MoveOnly> A;
+  MoveOnly::ResetCounts();
+  A.emplace(4);
+  EXPECT_TRUE((bool)A);
+  EXPECT_EQ(4, A->val);
+  EXPECT_EQ(0u, MoveOnly::MoveConstructions);
+  EXPECT_EQ(0u, MoveOnly::MoveAssignments);
+  EXPECT_EQ(0u, MoveOnly::Destructions);
+}
+
 #if LLVM_HAS_RVALUE_REFERENCE_THIS
 
 TEST_F(OptionalTest, MoveGetValueOr) {