[libcxx-commits] [PATCH] D102135: [libcxx][ranges] adds _`copyable-box`_

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver added a comment.

This looks good to me sans the one small comment.



================
Comment at: libcxx/include/__ranges/copyable_box.h:51
+  class __copyable_box {
+    std::optional<_Tp> __val_;
+
----------------
`std::`


================
Comment at: libcxx/include/__ranges/copyable_box.h:51
+  class __copyable_box {
+    std::optional<_Tp> __val_;
+
----------------
zoecarver wrote:
> `std::`
Just curious: why is having a member better than inheriting? 


================
Comment at: libcxx/test/libcxx/ranges/range.adaptors/range.copy.wrap/assign.move.pass.cpp:112
+  static_assert(test());
+
+  // Tests for the empty state. Those can't be constexpr, since they are only reached
----------------
You're going to need an `#if exceptions-enabled` here and in similar places in other tests. 


================
Comment at: libcxx/test/libcxx/ranges/range.adaptors/range.copy.wrap/properties.compile.pass.cpp:37
+// primary template
+static_assert(sizeof(std::ranges::__copyable_box<CopyConstructible>) > sizeof(CopyConstructible)); // must have an empty state
+
----------------
I might argue that we don't really need this line. But I suppose it doesn't really hurt. 

(The reason being, we don't really care if these are equal in size, we care if the throwing behavior works.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102135/new/

https://reviews.llvm.org/D102135