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

Jordan Rose jordan_rose at apple.com
Tue Sep 30 09:34:11 PDT 2014


================
Comment at: include/llvm/ADT/Optional.h:86
@@ +85,3 @@
+  /// Create a new object by default-constructing it in place.
+  void emplace() {
+    reset();
----------------
dblaikie wrote:
> 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?
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.

================
Comment at: unittests/ADT/OptionalTest.cpp:182
@@ +181,3 @@
+  int x, y;
+  MultiArgConstructor(int x, int y) : x(x), y(y) {}
+};
----------------
dblaikie wrote:
> dblaikie wrote:
> > 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))
> 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)
Good point, will do.

================
Comment at: unittests/ADT/OptionalTest.cpp:182
@@ +181,3 @@
+  int x, y;
+  MultiArgConstructor(int x, int y) : x(x), y(y) {}
+};
----------------
jordan_rose wrote:
> dblaikie wrote:
> > dblaikie wrote:
> > > 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))
> > 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)
> Good point, will do.
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.

http://reviews.llvm.org/D5508






More information about the llvm-commits mailing list