[libcxx-commits] [PATCH] D105816: [libc++] Implement views::all_t and ranges::viewable_range

[Louis Dionne via Phabricator via libcxx-commits]

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


================
Comment at: libcxx/test/std/ranges/range.req/range.refinements/viewable_range.compile.pass.cpp:108
+static_assert(std::ranges::range<T7&>);
+static_assert(!std::ranges::view<T7>);
+static_assert(!std::constructible_from<T7, T7&>);
----------------
Quuxplusone wrote:
> ldionne wrote:
> > Quuxplusone wrote:
> > > `s/T7/T7&/` here
> > Hmm, are you sure about that? In all those places, I'm checking whether `ranges::view<remove_cvref_t<T>>` is satisfied. Here, `remove_cvref_t<T>` is `T7`, not `T7&`.
> Ah, okay, I see what you're doing now. This is subtler than I'd like. I started to suggest that you might like to rewrite all 8 test cases in the form
> ```
> {
>   struct Tn { ... };
>   using X = Tn;  // or Tn& as appropriate
>   static_assert([! ]std::ranges::range<X>);
>   static_assert([! ]std::ranges::view<std::remove_cvref_t<X>>);
>   static_assert([! ]std::constructible_from<std::remove_cvref_t<X>, X>>);
>   static_assert([! ]std::borrowed_range<X>>);
>   static_assert([! ]std::viewable_range<X>>);
> }
> ```
> However, this is troublesome for the types like T2 and T2 that need to specialize `std::enable_borrowed_range`; you can't do that with a function-local struct type. So I have no great suggestion here.
> 
> Btw, does at least one of these tests fail when you go into `<__ranges/concepts.h>` and change each instance of `remove_cvref_t` to `remove_reference_t`? How about `decay_t`?
> Ah, okay, I see what you're doing now. This is subtler than I'd like.

I agree. I thought about doing something like what you suggest, but I encountered issues with needing to specialize `borrowed_range` that made the whole thing difficult to digest.

> Btw, does at least one of these tests fail when you go into `<__ranges/concepts.h>` and change each instance of `remove_cvref_t` to `remove_reference_t`? 

Yes, except for the `constructible_from` requirement, where changing from `remove_cvref_t` to `remove_reference_t` doesn't make a difference.

> How about `decay_t`?

No, nothing fails. However, `decay` differs from `remove_cvref_t` in that it turns arrays into pointers, and functions into pointers-to-functions. In both cases, the original types (before `decay_t`) won't satisfy `viewable_range`, and wouldn't satisfy `viewable_range` after `decay_t`. So I don't think it's observable -- we could use `decay_t` in the implementation and we'd still be conforming. LMK if you think I'm mistaken.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105816