[libcxx-commits] [PATCH] D104262: [libcxx][ranges] Add contiguous_range.

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver added inline comments.


================
Comment at: libcxx/test/std/ranges/range.req/range.refinements/contiguous_range.compile.pass.cpp:42-46
+struct BadDataMember {
+  int *begin();
+  int *end();
+  char *data();
+};
----------------
Quuxplusone wrote:
> cjdb wrote:
> > zoecarver wrote:
> > > cjdb wrote:
> > > > Quuxplusone wrote:
> > > > > I was initially confused by the name, as this class has //no// data members... but then I saw: the trick is that `X::data()` member function has a return type that is different from `std::add_pointer_t<ranges::range_reference_t<X>>`.
> > > > > I think it'd be worth replacing this test (or at augmenting it) with both of these:
> > > > > ```
> > > > > struct WrongConstness {
> > > > >     const int *begin() const;
> > > > >     const int *end() const;
> > > > >     int *data() const;
> > > > > };
> > > > > static_assert(std::ranges::random_access_range<WrongConstness>);
> > > > > static_assert(!std::ranges::contiguous_range<WrongConstness>);
> > > > > 
> > > > > struct NotPtr {
> > > > >     contiguous_iterator<const int*> begin() const;
> > > > >     contiguous_iterator<const int*> end() const;
> > > > >     contiguous_iterator<const int*> data() const;
> > > > > };
> > > > > static_assert(std::ranges::random_access_range<NotPtr>);
> > > > > static_assert(!std::ranges::contiguous_range<NotPtr>);
> > > > > ```
> > > > > I was initially confused by the name, as this class has //no// data members... but then I saw: the trick is that `X::data()` member function has a return type that is different from `std::add_pointer_t<ranges::range_reference_t<X>>`.
> > > > 
> > > > I propose renaming to `BadDataMemberFunction`.
> > > > 
> > > > We should also have a check for when a range can satisfy `contiguous_range<R>`, but not `contiguous_range<R const>`.
> > > Thanks for the suggestions. Arthur, I added your first test only (with some modification). The `NotPtr` test doesn't work because contiguous_iterator has an element_type. 
> > This still isn't complete, because now we're missing a test case for when both `contiguous_range<T>` and `contiguous_range<T const>` are true, and one for when only `contiguous_range<T const>` is true.
> > Arthur, I added your first test only (with some modification). The NotPtr test doesn't work because contiguous_iterator has an element_type.
> 
> Ah. My intent with `NotPtr` was that `contiguous_range<NotPtr>` would not be satisfied because `NotPtr().data()`'s return type is not `std::same_as<std::add_pointer_t<ranges::range_reference_t<T>>>`. However, I see now that the thing-we're-taking-the-return-type-of in [contiguous_range](https://en.cppreference.com/w/cpp/ranges/contiguous_range) is not `NotPtr().data()` but instead `std::ranges::data(NotPtr())`, which [actually evaluates](https://en.cppreference.com/w/cpp/ranges/data) to `std::to_address(NotPtr().begin())`, not `NotPtr().data()`, in this case.
> 
> So then actually maybe it would be worthwhile to add a test like this:
> ```
> struct WrongObjectness { // https://godbolt.org/z/M7fbvj7fE
>     const int *begin() const;
>     const int *end() const;
>     void *data() const;
> };
> static_assert(std::ranges::contiguous_range<WrongObjectness>);
> ```
> which verifies that if `data()` exists but is completely the wrong return type, then there's no problem at all. (Did I ever mention how overcomplicated Ranges is? :P)
> when both contiguous_range<T> and contiguous_range<T const> are true,

That's `test_range<contiguous_iterator>`.

>  and one for when only contiguous_range<T const> is true.

Added.


================
Comment at: libcxx/test/std/ranges/range.req/range.refinements/contiguous_range.compile.pass.cpp:42-46
+struct BadDataMember {
+  int *begin();
+  int *end();
+  char *data();
+};
----------------
zoecarver wrote:
> Quuxplusone wrote:
> > cjdb wrote:
> > > zoecarver wrote:
> > > > cjdb wrote:
> > > > > Quuxplusone wrote:
> > > > > > I was initially confused by the name, as this class has //no// data members... but then I saw: the trick is that `X::data()` member function has a return type that is different from `std::add_pointer_t<ranges::range_reference_t<X>>`.
> > > > > > I think it'd be worth replacing this test (or at augmenting it) with both of these:
> > > > > > ```
> > > > > > struct WrongConstness {
> > > > > >     const int *begin() const;
> > > > > >     const int *end() const;
> > > > > >     int *data() const;
> > > > > > };
> > > > > > static_assert(std::ranges::random_access_range<WrongConstness>);
> > > > > > static_assert(!std::ranges::contiguous_range<WrongConstness>);
> > > > > > 
> > > > > > struct NotPtr {
> > > > > >     contiguous_iterator<const int*> begin() const;
> > > > > >     contiguous_iterator<const int*> end() const;
> > > > > >     contiguous_iterator<const int*> data() const;
> > > > > > };
> > > > > > static_assert(std::ranges::random_access_range<NotPtr>);
> > > > > > static_assert(!std::ranges::contiguous_range<NotPtr>);
> > > > > > ```
> > > > > > I was initially confused by the name, as this class has //no// data members... but then I saw: the trick is that `X::data()` member function has a return type that is different from `std::add_pointer_t<ranges::range_reference_t<X>>`.
> > > > > 
> > > > > I propose renaming to `BadDataMemberFunction`.
> > > > > 
> > > > > We should also have a check for when a range can satisfy `contiguous_range<R>`, but not `contiguous_range<R const>`.
> > > > Thanks for the suggestions. Arthur, I added your first test only (with some modification). The `NotPtr` test doesn't work because contiguous_iterator has an element_type. 
> > > This still isn't complete, because now we're missing a test case for when both `contiguous_range<T>` and `contiguous_range<T const>` are true, and one for when only `contiguous_range<T const>` is true.
> > > Arthur, I added your first test only (with some modification). The NotPtr test doesn't work because contiguous_iterator has an element_type.
> > 
> > Ah. My intent with `NotPtr` was that `contiguous_range<NotPtr>` would not be satisfied because `NotPtr().data()`'s return type is not `std::same_as<std::add_pointer_t<ranges::range_reference_t<T>>>`. However, I see now that the thing-we're-taking-the-return-type-of in [contiguous_range](https://en.cppreference.com/w/cpp/ranges/contiguous_range) is not `NotPtr().data()` but instead `std::ranges::data(NotPtr())`, which [actually evaluates](https://en.cppreference.com/w/cpp/ranges/data) to `std::to_address(NotPtr().begin())`, not `NotPtr().data()`, in this case.
> > 
> > So then actually maybe it would be worthwhile to add a test like this:
> > ```
> > struct WrongObjectness { // https://godbolt.org/z/M7fbvj7fE
> >     const int *begin() const;
> >     const int *end() const;
> >     void *data() const;
> > };
> > static_assert(std::ranges::contiguous_range<WrongObjectness>);
> > ```
> > which verifies that if `data()` exists but is completely the wrong return type, then there's no problem at all. (Did I ever mention how overcomplicated Ranges is? :P)
> > when both contiguous_range<T> and contiguous_range<T const> are true,
> 
> That's `test_range<contiguous_iterator>`.
> 
> >  and one for when only contiguous_range<T const> is true.
> 
> Added.
> So then actually maybe it would be worthwhile to add a test like this:

I considered adding a test like this. I don't see how it's really different than the const tests (in both cases it fails because the types are not the same). I think it would be hard to write an implementation that said "if this is the same return type with different constness, that's bad, but if it's a totally unrelated types, that's OK."

I suppose it wouldn't hurt to have a case where they both returned const pointers. I'll add a similar test but where the return type is `const char *`. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104262