[PATCH] Add an emplace(...) method to llvm::Optional<T>.
David Blaikie
dblaikie at gmail.com
Tue Sep 30 09:27:38 PDT 2014
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)
================
Comment at: include/llvm/ADT/Optional.h:86
@@ +85,3 @@
+ /// Create a new object by default-constructing it in place.
+ void emplace() {
+ reset();
----------------
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?
================
Comment at: unittests/ADT/OptionalTest.cpp:182
@@ +181,3 @@
+ int x, y;
+ MultiArgConstructor(int x, int y) : x(x), y(y) {}
+};
----------------
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))
================
Comment at: unittests/ADT/OptionalTest.cpp:182
@@ +181,3 @@
+ int x, y;
+ MultiArgConstructor(int x, int y) : x(x), y(y) {}
+};
----------------
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)
http://reviews.llvm.org/D5508
More information about the llvm-commits
mailing list