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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone 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>;
+
----------------
ldionne wrote:
> 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).
+1 @ldionne. Also, there's certainly no need to introduce multiple subsuming concepts here. Subsumption is a tool for solving a problem (namely, concept overloading); we should use the tool if-and-only-if we are solving that problem (or of course if the paper standard is mandating us to do so).


================
Comment at: libcxx/include/__ranges/copyable_box.h:47
+  class __copyable_box {
+    optional<_Tp> __val_;
+
----------------
ldionne wrote:
> cjdb wrote:
> > Can this be `[[no_unique_address]]`?
> There's really no point in doing that AFAICT, because `std::optional` is never empty.
`[[no_unique_address]]` also has a layout effect on non-empty types, if they're tail-padded: https://godbolt.org/z/qjMWo699x

However, AIUI, its effect never leaks out of a type altogether: it makes a difference //only// on a member of `X` that is directly followed by another member of `X`. So if you have a class with only one data member, like this version of `__copyable_box`, then adding `[[no_unique_address]]` to that unique data member will never make a difference. (I think.) https://godbolt.org/z/K3GPPaxbz


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102135