Page MenuHomePhabricator

[libc++] Implement views::all_t and ranges::viewable_range
ClosedPublic

Authored by ldionne on Jul 12 2021, 7:45 AM.

Details

Diff Detail

Event Timeline

ldionne requested review of this revision.Jul 12 2021, 7:45 AM
ldionne created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJul 12 2021, 7:45 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Quuxplusone accepted this revision as: Quuxplusone.Jul 12 2021, 9:18 AM
Quuxplusone added a subscriber: Quuxplusone.

LGTM % comments.

libcxx/test/std/ranges/range.adaptors/range.all/all_t.compile.pass.cpp
35–36

I'd test BorrowableRange& and BorrowableRange const& right here, as well.

libcxx/test/std/ranges/range.req/range.refinements/viewable_range.compile.pass.cpp
109

s/T7/T7&/ here

115
struct T8 : test_range<cpp20_input_iterator> {
  T8(T8 const&) = delete;
};
static_assert(std::ranges::range<T8>);
static_assert(!std::ranges::view<T8>);
static_assert(!std::constructible_from<T8, T8>);
static_assert(!std::ranges::borrowed_range<T8>);

static_assert(!std::ranges::viewable_range<T8>);
120

Maybe test int[] and int[10] as well?

ldionne marked 3 inline comments as done.Jul 13 2021, 6:42 PM
ldionne added inline comments.
libcxx/test/std/ranges/range.req/range.refinements/viewable_range.compile.pass.cpp
109

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&.

115

Indeed 😅. I guess I got thinking too hard and missed the obvious.

ldionne updated this revision to Diff 358489.Jul 13 2021, 6:42 PM
ldionne marked an inline comment as done.

Address Arthur's comments.

Quuxplusone added inline comments.Jul 13 2021, 8:34 PM
libcxx/test/std/ranges/range.req/range.refinements/viewable_range.compile.pass.cpp
109

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?

ldionne marked 2 inline comments as done.Jul 14 2021, 9:25 AM
ldionne added inline comments.
libcxx/test/std/ranges/range.req/range.refinements/viewable_range.compile.pass.cpp
109

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.

ldionne updated this revision to Diff 358640.Jul 14 2021, 9:25 AM
ldionne marked an inline comment as done.

Try to mitigate Arthur's concerns

ldionne updated this revision to Diff 358683.Jul 14 2021, 11:48 AM

Rebase onto main for CI.

ldionne updated this revision to Diff 358704.Jul 14 2021, 12:51 PM

Rebase onto main for CI.

ldionne accepted this revision.Jul 15 2021, 4:54 AM
This revision is now accepted and ready to land.Jul 15 2021, 4:54 AM