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

Tue Jul 6 14:14:02 PDT 2021

ldionne marked 3 inline comments as done.
ldionne added a comment.

I'm not seeing any more blocking comments here. I'll ship this tomorrow morning unless someone raises a blocking comment.

We can lift `__copy_constructible_object` into a common header (`<concepts>`?) in the `transform_view` review or later as a NFC consolidation commit. I don't think it's worth blocking this review over that issue.

================
Comment at: libcxx/include/__ranges/copyable_box.h:40-41
+
+template<class _Tp>
+concept __copy_constructible_object = copy_constructible<_Tp> && is_object_v<_Tp>;
+
----------------
Quuxplusone wrote:
> cjdb wrote:
> > ldionne wrote:
> > > 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.
> > If the goalpost is going to be moved, let's get rid of it here too. The standard doesn't say anything about `__copy_constructible_object` at all, and if we're going to limit ourselves to only standard-provided exposition-only detail concepts: you're breaking ranks here.
> > 
> > In other words: this decision is arbitrary and inconsistent.
> Philosophically @cjdb you're right, let's eliminate the concept. But physically I don't think we //can//, because we need to both "constrain `__copyable_box`" //and// "provide a partial specialization of it," which means we need to repeat the primary template's constraint in two different places (currently lines 45 and 115), and for that we seem to //need// to use a concept, because if we simply repeated `requires is_object_v<_Tp>` in both places, that would be a different atomic constraint every place it lexically appeared. So we //need// to factor it out into a concept...
> 
> ...except, right, why is `__copyable_box` constrained at all?! It's an implementation detail of libc++! It doesn't need to be constrained; the constraint will be applied way up at the `transform_view` or whatever. Constraining doesn't make sense at this level, because there's no "fallback"; there's no reason `__copyable_box<Foo>` needs to be SFINAE-friendly ill-formed, because we're never going to SFINAE on it. So you can actually just remove the constraint entirely. This fixes the complaint about the concept (no more concept), and also simplifies the code around that partial specialization.
> 
> Maybe add `static_assert(std::is_object_v<_Tp>);` inside the class definition, just for sanity-checking. We should never fail that assertion.
We're not limiting ourselves to exposition only concepts mentioned in the Standard as any kind of policy. I'm just saying it makes no sense to lift it into `<concepts>` before we even have a use for it somewhere else, since we *do* only have the exposition-only concepts mentioned in the Standard inside `<concepts>` so far.

If we do have a use for the concept elsewhere (`transform_view` seems to need it at the very least), and if it's conforming to use it there (I think it is), then I believe it would be reasonable to lift the concept into `<concepts>` and use it in `transform_view` (and wherever else makes sense).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102135