Details
- Reviewers
ldionne • Quuxplusone cjdb - Group Reviewers
Restricted Project - Commits
- rG34503987385b: [libcxx][ranges] Add contiguous_range.
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
LGTM % comments.
libcxx/test/std/containers/sequences/deque/range_concept_conformance.compile.pass.cpp | ||
---|---|---|
26 | deque<T> is not a contiguous range. Add ! here and below. | |
libcxx/test/std/containers/sequences/vector.bool/range_concept_conformance.compile.pass.cpp | ||
26 | vector<bool> is not a contiguous range. Add ! here and below. | |
libcxx/test/std/ranges/range.req/range.refinements/contiguous_range.compile.pass.cpp | ||
15 | contiguous_range | |
43–47 | 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>>. 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>); | |
libcxx/test/std/ranges/range.req/range.refinements/subsumption.compile.pass.cpp | ||
28 | Whitespace: Please indent requires by one level (here and throughout). E.g. template<...> requires ... bool foo() { return ... } It'd be nice to remove the [[nodiscard]]s while you're at it. |
libcxx/test/std/ranges/range.req/range.refinements/contiguous_range.compile.pass.cpp | ||
---|---|---|
43–47 |
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>. | |
libcxx/test/std/ranges/range.req/range.refinements/subsumption.compile.pass.cpp | ||
28 | @ldionne left it up to me to decide how libc++ formats requires clauses, and I decided that they shouldn't be indented. |
Can we also remove the TODO in ref_view::data() in this patch?
libcxx/include/__ranges/concepts.h | ||
---|---|---|
1 | We're missing a few concepts in the synopsis in <ranges>. I think we're only missing random_access_range and contiguous_range -- can you update that part of the synopsis in this patch? |
libcxx/include/__ranges/concepts.h | ||
---|---|---|
1 | Hmm, the two I implemented... 🤔 | |
libcxx/test/std/ranges/range.req/range.refinements/contiguous_range.compile.pass.cpp | ||
43–47 | 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. | |
libcxx/test/std/ranges/range.req/range.refinements/subsumption.compile.pass.cpp | ||
28 | I'm going to match the existing format of this file. I would support a nfc commit that updated all the requires in this file. |
libcxx/test/std/ranges/range.req/range.refinements/contiguous_range.compile.pass.cpp | ||
---|---|---|
43–47 | 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. | |
libcxx/test/std/ranges/range.req/range.refinements/subsumption.compile.pass.cpp | ||
28 | Let's not take any action till Louis and I resolve our discussion. |
libcxx/test/std/ranges/range.req/range.refinements/contiguous_range.compile.pass.cpp | ||
---|---|---|
43–47 |
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 is not NotPtr().data() but instead std::ranges::data(NotPtr()), which actually evaluates 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) |
Please resolve the concerns about testing raised by other reviewers, but apart from that, LGTM.
Rebase. Update based on review.
Will be landed when CI is green.
libcxx/test/std/ranges/range.req/range.refinements/contiguous_range.compile.pass.cpp | ||
---|---|---|
43–47 |
That's test_range<contiguous_iterator>.
Added. | |
43–47 |
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 *. |
libcxx/test/std/ranges/range.req/range.refinements/contiguous_range.compile.pass.cpp | ||
---|---|---|
43–47 | Wait, on this latest point I think I'm right and you're confused. My WrongObjectness test above is supposed to pass, not fail, even though the return type of data is not the same as the return type of begin — because it is so different that we wrap around and become a contiguous range again! (Try the godbolt link.) |
libcxx/test/std/ranges/range.req/range.refinements/contiguous_range.compile.pass.cpp | ||
---|---|---|
43–47 | Oh, interesting. I see (because ranges::data won't pick the data member function). Sorry I didn't see this comment until I landed the patch. I'll make a follow up commit. |
We're missing a few concepts in the synopsis in <ranges>. I think we're only missing random_access_range and contiguous_range -- can you update that part of the synopsis in this patch?