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

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver added inline comments.


================
Comment at: libcxx/include/__ranges/copyable_box.h:31
+
+#if _LIBCPP_STD_VER > 17 && !defined(_LIBCPP_HAS_NO_RANGES)
+
----------------
Do we need both of these conditions? I thought that the latter subsumed the former.


================
Comment at: libcxx/include/__ranges/copyable_box.h:52
+
+    constexpr __copyable_box& operator=(const __copyable_box& __other)
+    noexcept(is_nothrow_copy_constructible_v<_Tp>)
----------------
Is this missing the "If copyable<T> is not modeled..."?


================
Comment at: libcxx/include/__ranges/copyable_box.h:68
+
+    constexpr __copyable_box& operator=(__copyable_box&& __other)
+    noexcept(is_nothrow_move_constructible_v<_Tp>)
----------------
Same as above.


================
Comment at: libcxx/test/libcxx/ranges/range.adaptors/range.copy.wrap/base.compile.pass.cpp:15
+
+#include <__ranges/copyable_box.h>
+
----------------
Hmm, I know this is a libc++ only test, but I still think maybe it should include ranges. (Same elsewhere.)


================
Comment at: libcxx/test/libcxx/ranges/range.adaptors/range.copy.wrap/default_initializable.pass.cpp:67
+  check_default_initialization<not_default_initializable>();
+
+  return 0;
----------------
What about a type that had a noexcept(false) default ctor?


================
Comment at: libcxx/test/libcxx/ranges/range.adaptors/range.copy.wrap/properties.compile.pass.cpp:20
+
+static_assert(std::derived_from<std::ranges::__copyable_box<int>, std::optional<int> >);
+
----------------
Do you really need the base test if you have this?


================
Comment at: libcxx/test/libcxx/ranges/range.adaptors/range.copy.wrap/properties.compile.pass.cpp:30
+
+struct not_copy_constructible {
+  not_copy_constructible() = default;
----------------
Can you rename this to `move_only` or something? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102135