There are no changes to public APIs.
Details
- Reviewers
ldionne cjdb • Quuxplusone - Group Reviewers
Restricted Project - Commits
- rG89599e8b201a: [libcxx][ranges] Add concepts in range.utility.helpers.
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
LGTM % comments.
libcxx/include/__iterator/concepts.h | ||
---|---|---|
172–173 | Nit: Please use _Ip not _Iter, for consistency. | |
libcxx/include/__iterator/iterator_traits.h | ||
257–258 | Before this change: C++20 uses the name has-arrow for something complicated; libc++ uses __has_arrow for a subtly different thing. This is obviously awful and I'm glad you're changing it. I think the right thing to do here is actually eliminate __has_arrow_op altogether. We use it in only one place. It's no hardship to inline it on line 264: template<class _Ip> requires (requires(_Ip& __i) { __i.operator->(); }) && (!__has_member_pointer<_Ip>) As a drive-by, you might also rename __iterator_traits_member_pointer_or_arrow_or_void to simply __iterator_traits_member_pointer, for consistency with __iterator_traits_member_reference. | |
libcxx/test/libcxx/ranges/range.utility.helpers/has_arrow.compile.pass.cpp | ||
78 | This surprises me, but I suppose it's because Incomplete* is not an input iterator, because ++it is ill-formed, because Incomplete is incomplete? | |
libcxx/test/libcxx/ranges/range.utility.helpers/not_same.compile.pass.cpp | ||
16–22 | As I think @tcanens remarked a few days ago: WG21 has an exposition-only concept not-same-as that is not the same thing as not same_as? That's just awful. If you keep this test, you should add static_assert(!std::__not_same_as<int, int&>); static_assert(!std::__not_same_as<int&, const int&>); static_assert(!std::__not_same_as<int(&)(), int()>); static_assert(std::__not_same_as<int(&)(), int(*)()>); i.e., test some things where the ref-qualification is on the right-hand operand; and test some things where remove_cvref_t is visibly different from decay_t. | |
libcxx/test/libcxx/ranges/range.utility.helpers/simple_view.compile.pass.cpp | ||
23 | I suggest calling this WrongConstView or something, since if you ever saw this in real life it would be a bug. | |
30–33 | I suggest you add a positive example OnlyConstView, with only friend const int* begin(const OnlyConstView&). |
libcxx/test/libcxx/ranges/range.utility.helpers/not_same.compile.pass.cpp | ||
---|---|---|
16–22 | I agree the name/concept is terrible. But it is widely used throughout the ranges library, so I think we're stuck with it for a little while. Additionally, I think it's imperative that we continue to name exposition-only-concepts in a mechanical way: __exposition_only_concepts. This way we can easily lookup concepts, verify correctness, and prevent duplication. I'll add those tests. |
libcxx/test/libcxx/ranges/range.utility.helpers/has_arrow.compile.pass.cpp | ||
---|---|---|
78 | Yes. | |
libcxx/test/libcxx/ranges/range.utility.helpers/simple_view.compile.pass.cpp | ||
30–33 | This isn't a view. A view requires that T is movable (i.e., not const) and that we can do ranges::begin(declval<T&>()). Therefor, we need all four members. |
LGTM
libcxx/test/libcxx/ranges/range.utility.helpers/simple_view.compile.pass.cpp | ||
---|---|---|
36–39 | This type isn't necessary: test_iterators.h has a sentinel_wrapper vocabulary test type that we can use instead. |
libcxx/test/libcxx/ranges/range.utility.helpers/simple_view.compile.pass.cpp | ||
---|---|---|
30–33 | @zoecarver: I believe you've misunderstood. I'm asking that you add this test case: struct OnlyConstView : std::ranges::view_base { friend int* begin(const OnlyConstView&); friend int* end(const OnlyConstView&); }; static_assert(std::ranges::__simple_view<OnlyConstView>); And then modify DifferentSentinel as follows: struct DifferentSentinel : std::ranges::view_base { friend int* begin(DifferentSentinel const&); friend sentinel_wrapper<int*> end(DifferentSentinel const&); }; static_assert(!std::ranges::__simple_view<DifferentSentinel>); |
libcxx/test/libcxx/ranges/range.utility.helpers/simple_view.compile.pass.cpp | ||
---|---|---|
30–33 | No, I understood you. Maybe the following snippet will help me explain what I'm trying to say: struct OnlyConstView : std::ranges::view_base { friend int* begin(const OnlyConstView&); friend int* end(const OnlyConstView&); }; static_assert(!std::ranges::range<OnlyConstView>); static_assert( std::ranges::range<const OnlyConstView>); static_assert(!std::ranges::view<OnlyConstView>); static_assert(!std::ranges::view<const OnlyConstView>); static_assert(!std::ranges::__simple_view<OnlyConstView>); static_assert(!std::ranges::__simple_view<const OnlyConstView>); Here, you can see that only const OnlyConstView is a range. This is because a range requires us to be able to do ranges::begin(declval<T&>()) (so T must be const). You'll also notice that in no case is this a view, because a view requires T to be movable, so it cannot be const. And __simple_view requires T to be a view, so this ins't a __simple_view either. |
libcxx/test/libcxx/ranges/range.utility.helpers/simple_view.compile.pass.cpp | ||
---|---|---|
30–33 | Ahhh. This is https://cplusplus.github.io/LWG/issue3480 — the poison pill for ranges::begin is "bad" in the sense that begin(auto&) // poison-pill is a better-matching candidate than our actual ADL begin(const OnlyConstView&). struct ValueView : std::ranges::view_base { friend int* begin(ValueView); friend int* end(ValueView); }; static_assert( std::ranges::range<ValueView>); static_assert( std::ranges::range<const ValueView>); static_assert( std::ranges::view<ValueView>); static_assert(!std::ranges::view<const ValueView>); // because const types are not assignable-to static_assert( std::ranges::__simple_view<ValueView>); However, if you think this is too far from the original intent of the test, I guess I'm ambivalent at this point. It basically depends on whether we think "pass views by value" is a good general rule. |
libcxx/test/libcxx/ranges/range.utility.helpers/not_same.compile.pass.cpp | ||
---|---|---|
16–22 | not-same-as has been editorially renamed to different-from: https://github.com/cplusplus/draft/commit/caf7428fd6c807f30538042cf9bf1b27b8772b01 |
Nit: Please use _Ip not _Iter, for consistency.