[libc++][ranges] Implement `lazy_split_view`.

[var-const]

Summary

Note that this class was called just split_view in the original One
Ranges Proposal and was renamed to lazy_split_view by
P2210.

Original patch by @zoecarver.

[var-const]

Add the forgotten CPO test and try to fix the CI.

[var-const]

Rebase on main.

[var-const]

Fix the CI, pt. 2.

[ldionne]

Thanks a lot! This LGTM with my comments applied and CI passing.

libcxx/include/__ranges/lazy_split_view.h

4

You should rebase on top of 9a44ed43cf9a8d4e3e55d007432e363e094ec6b0. Basically, rm -rf libcxx/test/libcxx/diagnostics/detail.headers and then regenerate auto-generated files.

libcxx/test/std/ranges/range.adaptors/range.lazy.split/base.pass.cpp

41–44

I've actually had the same discussion with @philnik and he told me he was basically testing the test, which I had not realized at first. I think this is great. However, I think it should be sufficient to have just a single positive case to confirm that CanCallBase works as intended, and then the other ones can be runtime tests instead.

However, here it looks like the const&& and the const& variants don't have runtime tests below -- so we should add some and then remove the positive static_asserts corresponding to them above.

libcxx/test/std/ranges/range.adaptors/range.lazy.split/range.lazy.split.inner/assert.equal.pass.cpp

9 ↗

(On Diff #419930)

This should also be unsupported in C++11, C++14, C++17, because we are using ranges.

10 ↗

(On Diff #419930)

This should be -D_LIBCPP_ENABLE_ASSERTIONS=1. Please apply to other assertion tests too!

libcxx/test/support/is_explicit_ctr.h

15 ↗

(On Diff #419930)

Would it be sufficient to use !std::is_convertible<T, U> && std::is_constructible<T, U>? At that point, you might not need this helper at all, since you can probably just query !std::is_convertible<T, U> in your tests (constructibility is already tested by the fact that you construct objects in your tests).

[var-const]

Address feedback, try to fix the CI.

[var-const]

Thanks a lot for the review!

libcxx/test/std/ranges/range.adaptors/range.lazy.split/range.lazy.split.inner/assert.equal.pass.cpp

10 ↗

(On Diff #419930)

Couldn't find any other hits, which is weird because I'm sure I copied this from some other assertion test. Maybe it got fixed recently.

libcxx/test/support/is_explicit_ctr.h

15 ↗

(On Diff #419930)

Done.

The only thing is that IsExplicitCtr supports any number of arguments (i.e., also supports multi-argument constructors and default constructors). However, I don't think the Standard ever deliberately applies explicit to anything other than single-argument constructors, so this functionality is probably unnecessary (unless we want to be particularly paranoid and check every constructor for being non-explicit unless specified so).

[var-const]

Try to fix GCC.

[philnik]

Other than moving the libc++-specific tests in the libcxx/ subdirectory all the comments are very nit-picky, so this LGTM!

libcxx/test/std/ranges/range.adaptors/range.lazy.split/begin.pass.cpp

28

I looks like you aren't checking that begin() caches the result. Could you add a test for that?

libcxx/test/std/ranges/range.adaptors/range.lazy.split/ctor.default.pass.cpp

42–50

Why do you have effectively the same test twice?

libcxx/test/std/ranges/range.adaptors/range.lazy.split/ctor.range.pass.cpp

26

It looks like you aren't checking that the constructor doesn't copy the range and pattern. Same for the view, pattern constructor.

30

What is decltype(auto) here? is it const char [8]? Assuming my guess is correct, is it possible to rewrite this as const char str[] = "abc def"? I think that would be clearer.

41–43

I think you should check here that lazy_split_view isn't happy with an input_range. I've got a few ForwardRangeNot* types in D121964. You can snack those if you want. It also looks like you aren't testing the constructible_from<V, views::all_t<R>> requirement. The first static_assert will fail at the forward_range.

libcxx/test/std/ranges/range.adaptors/range.lazy.split/ctor.view.pass.cpp

28–32

Is there a reason you aren't checking base() here?

libcxx/test/std/ranges/range.adaptors/range.lazy.split/range.lazy.split.inner/assert.equal.pass.cpp

1 ↗

(On Diff #420695)

Shouldn't these kinds of tests be in libcxx/...?

libcxx/test/std/ranges/range.adaptors/range.lazy.split/range.lazy.split.inner/ctor.default.pass.cpp

21

Could you check here that the base iterator is default constructed?

[var-const]

Add more friends to try to fix GCC.

[var-const]

Address feedback, try to fix GCC again.

[var-const]

libcxx/test/std/ranges/range.adaptors/range.lazy.split/begin.pass.cpp

28

Hmm, this is actually a very interesting question. My understanding was that this class uses non-propagating-cache for its "non-copying" behavior, not for actual caching, since the description of begin doesn't mention anything about caching, unlike e.g. filter_view::begin(). FWIW, neither GCC nor MSVC do caching here (if I'm reading their sources correctly).

However, using caching here only breaks a couple of tests that instantiate lazy_split_view with an InputView and expect begin() to return the beginning of the range after it has already been iterated on, which is likely an incorrect expectation for an input range.

Looking further, it looks like non-propagating-cache is defined as being equivalent to optional unless specified otherwise. The specification does not "override" operator=(const T&), which means it's equivalent to optional::operator=(const T&), and that does not perform any caching. So if I'm reading this correctly, this line:

current_ = ranges::begin(base_);

in the Standard does not by itself prescribe caching, and I don't see any additional requirements or remarks to change that.

@tcanens Can you please clarify whether caching behavior is intended in lazy_split_view::begin() on the current_ = ranges::begin(base_); line?

libcxx/test/std/ranges/range.adaptors/range.lazy.split/ctor.default.pass.cpp

42–50

I got this pattern from some other range tests we have. These are two different forms of initialization (default initialization vs. copy initialization), and the latter would fail to compile if the default constructor were explicit.

libcxx/test/std/ranges/range.adaptors/range.lazy.split/ctor.range.pass.cpp

26

Done. This required quite a bit of setup, PTAL.

30

Yes. Removed this occurrence and one other (in general.pass.cpp).

41–43

lazy_split_view supports input_ranges as long as Pattern is a tiny-range. I think these tests are covered by the tests in range.lazy.split/constraints.compile.pass.cpp, can you please check and see if you still notice something missing?

libcxx/test/std/ranges/range.adaptors/range.lazy.split/ctor.view.pass.cpp

28–32

InputView isn't comparable. Do you think it's important enough to define a ComparableInputView for this case? (it seems a little not worth it to me, but I'm open to do this if you feel it adds value)

libcxx/test/std/ranges/range.adaptors/range.lazy.split/range.lazy.split.inner/assert.equal.pass.cpp

1 ↗

(On Diff #420695)

Thanks for catching this!

libcxx/test/std/ranges/range.adaptors/range.lazy.split/range.lazy.split.inner/ctor.default.pass.cpp

21

I'm not sure there's a good way of doing that. If the underlying range is an input range, it will crash (base tries to access outer-iterator::parent, which is null for a default-constructed outer-iterator). It will work for a forward range, but my overall impression is that a default-constructed iterator isn't supposed to be used for anything other than reassignment.

[var-const]

Fix the CI (after moving the assert tests, they can no longer include "types.h").

[tcanens]

libcxx/test/std/ranges/range.adaptors/range.lazy.split/begin.pass.cpp

28

No. Caching input iterators doesn't make sense, and begin doesn't perform any non-constant time operation that requires caching anyway.

[ldionne]

libcxx/test/std/ranges/range.adaptors/range.lazy.split/begin.pass.cpp

28

I'm a bit confused. We do have __current_.__emplace(ranges::begin(__base_)); in our implementation for non-forward input-iterators. I think that's what @philnik was referring to as "caching", perhaps incorrectly. But we definitely store the result of ranges::begin in the view itself instead of in the iterators.

I guess it would make sense to have a test that would fail if we tried to store the result of ranges::begin(input-range) in the outer iterator itself, instead of storing it in __current_. I don't think we have one right now.

[tcanens]

libcxx/test/std/ranges/range.adaptors/range.lazy.split/begin.pass.cpp

28

Yes, you have to store it in the view, but it's not a cache for when begin is called again (which is not even valid); it's the state of the lazy algorithm.

[var-const]

Rebase on main, fix the CI.

[var-const]

More GCC workarounds for access control.

[var-const]

Fix the CI.

[var-const]

Rebase on main.

[var-const]

?

[var-const]

Undo accidental change.

[var-const]

libcxx/test/std/ranges/range.adaptors/range.lazy.split/begin.pass.cpp

28

@ldionne no_unique_address.compile.pass.cpp checks whether the underlying iterator is stored in outer-iterator or in the view itself. Re. caching, I think @philnik meant that we don't do something like if (!__current_.__has_value()) __current_.__emplace(...).

[philnik]

libcxx/test/std/ranges/range.adaptors/range.lazy.split/begin.pass.cpp

28

Just so you don't have to guess what I was talking about, I meant https://eel.is/c++draft/range.adaptors#range.split.view-4. Sorry for the confusion.

[var-const]

Fix the CI.

[var-const]

Try to fix the CI.

[var-const]

Fix the CI?

[var-const]

Fix the CI.

[var-const]

Fix the CI?

[var-const]

Fix the CI.