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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne marked 2 inline comments as done.
ldionne added inline comments.


================
Comment at: libcxx/include/__ranges/copyable_box.h:40-41
+
+template<class _Tp>
+concept __copy_constructible_object = copy_constructible<_Tp> && is_object_v<_Tp>;
+
----------------
cjdb wrote:
> Please move this to `<concepts>`: I plan to use this elsewhere! Also, it should be:
> ```
> template<class _Tp>
> concept destructible_object = destructible<_Tp> && is_object_v<_Tp>;
> 
> template<class _Tp>
> concept move_constructible_object = move_constructible<_Tp> && destructible_object<_Tp>;
> 
> template<class _Tp>
> concept copy_constructible_object = copy_constructible<_Tp> && move_constructible_object<_Tp>;
> ```
> (If we need `constructible_object_from`, I'll add that later.)
I suggest that we instead move it to `<concepts>` when we have that second use for it. As it is, it's really an implementation detail of `copyable_box`.

Also, http://eel.is/c++draft/range.copy.wrap#1.1 says:

> `copyable-box<T>` constrains its type parameter `T` with `copy_­constructible<T> && is_­object_­v<T>`.

`copy_­constructible<T>` already includes `destructible<T>` transitively (via `constructible_from`, at least).


================
Comment at: libcxx/include/__ranges/copyable_box.h:47
+  class __copyable_box {
+    optional<_Tp> __val_;
+
----------------
cjdb wrote:
> Can this be `[[no_unique_address]]`?
There's really no point in doing that AFAICT, because `std::optional` is never empty.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102135