[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