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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne marked 3 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:
> Quuxplusone wrote:
> > 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).
> Sure, I'll ask Zoe to move it in D103056, or when `single_view` is implemented.
> 
> > `copy_­constructible<T>` already includes `destructible<T>` transitively (via `constructible_from`, at least).
> 
> The point of my implementation is to create a subsumption hierarchy that doesn't eventually create a conflict between the groupings `destructible<T> && is_object_v<T>`, `move_constructible<T> && is_object_v<T>`, and `copy_constructible<T> && is_object_v<T>`. I don't want there to be a competing `__move_constructible_object` later on down the line, as I suspect there inevitably will be. Designing concepts is tricky business, and putting `is_object_v` here is arbitrary. It makes more sense to put it on the base concept and grow out, which is why `destructible` is at the base of all `constructible` concepts.
If the Standard had used some exposition-only concept `__copy_constructible_object` to constrain `F` in `transform_view`, then I'd agree strongly. As it is, I'm lukewarm. Let's lift it once we have a couple uses for it.


================
Comment at: libcxx/include/__ranges/copyable_box.h:47
+  class __copyable_box {
+    optional<_Tp> __val_;
+
----------------
Quuxplusone wrote:
> 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
That matches my understanding too. I don't think it's possible for the effect to leak out of the type since it must be possible to lay out objects of that type one after another in an array without messing up their alignment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102135