Page MenuHomePhabricator

[libcxx][ranges] Add concepts in range.utility.helpers.
ClosedPublic

Authored by zoecarver on Jun 1 2021, 2:55 PM.

Details

Summary

There are no changes to public APIs.

Diff Detail

Event Timeline

zoecarver requested review of this revision.Jun 1 2021, 2:55 PM
zoecarver created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJun 1 2021, 2:55 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript

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.
After this change: C++20 uses the name has-arrow; libc++ uses __has_arrow for the same thing; libc++ also uses __has_arrow_op for a subtly different thing. This is arguably a step in the right direction, but still awful, because now it subtly matters whether you say __has_arrow or __has_arrow_op.

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
15–21 ↗(On Diff #349105)

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.
Due to its awfulness, I would suggest either giving this a scarier name, or dropping it altogether if humanly possible. (And/or filing a LWG issue, of course.)

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.
Initially I thought SimpleView should be called Span (since it works like std::span), but that was when I was expecting the rest of the examples to be different positive examples (e.g. StringView) instead of negative examples.

30–33

I suggest you add a positive example OnlyConstView, with only friend const int* begin(const OnlyConstView&).
And then you can eliminate lines 40 and 42 from DifferentSentinel.

zoecarver added inline comments.Jun 1 2021, 4:01 PM
libcxx/test/libcxx/ranges/range.utility.helpers/not_same.compile.pass.cpp
15–21 ↗(On Diff #349105)

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.

zoecarver marked 2 inline comments as done.Jun 1 2021, 4:19 PM
zoecarver added inline comments.
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.

zoecarver updated this revision to Diff 349127.Jun 1 2021, 4:20 PM

Address review comments.

cjdb accepted this revision.Jun 1 2021, 4:40 PM

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.

Quuxplusone added inline comments.Jun 1 2021, 4:54 PM
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>);
zoecarver added inline comments.Jun 2 2021, 9:15 AM
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.

Quuxplusone accepted this revision as: Quuxplusone.Jun 2 2021, 10:04 AM
Quuxplusone added inline comments.
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&).
So the solution adopted in LWG3480 is to pass views by value, not by const reference — and arguably that's a more "classic STL" way to do it, anyway.
I'd replace this with:

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.

zoecarver updated this revision to Diff 349364.Jun 2 2021, 1:34 PM
  • Fix versioning.
  • Use test::sentinel_wrapper.
tcanens added inline comments.Jun 3 2021, 5:25 PM
libcxx/test/libcxx/ranges/range.utility.helpers/not_same.compile.pass.cpp
15–21 ↗(On Diff #349105)

not-same-as has been editorially renamed to different-from: https://github.com/cplusplus/draft/commit/caf7428fd6c807f30538042cf9bf1b27b8772b01

zoecarver marked 8 inline comments as done.Jun 4 2021, 9:52 AM

I'm going to do the rename and land.

libcxx/test/libcxx/ranges/range.utility.helpers/not_same.compile.pass.cpp
15–21 ↗(On Diff #349105)

Woohoo 🙏

Thanks @tcanens (and others).

This revision was not accepted when it landed; it landed in state Needs Review.Jun 4 2021, 9:56 AM
This revision was automatically updated to reflect the committed changes.
zoecarver marked an inline comment as done.