Page MenuHomePhabricator

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

Authored by var-const on Aug 4 2021, 2:43 PM.

Details

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.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
var-const updated this revision to Diff 419898.Apr 1 2022, 5:14 PM

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

var-const updated this revision to Diff 419899.Apr 1 2022, 5:16 PM

Rebase on main.

var-const updated this revision to Diff 419930.Apr 1 2022, 9:15 PM

Fix the CI, pt. 2.

ldionne accepted this revision.Apr 5 2022, 10:59 AM

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 updated this revision to Diff 420669.Apr 5 2022, 5:25 PM
var-const marked 6 inline comments as done.

Address feedback, try to fix the CI.

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 updated this revision to Diff 420695.Apr 5 2022, 8:29 PM

Try to fix GCC.

philnik accepted this revision.Apr 7 2022, 2:06 AM

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?

This revision is now accepted and ready to land.Apr 7 2022, 2:06 AM
var-const updated this revision to Diff 421387.Apr 7 2022, 7:19 PM

Add more friends to try to fix GCC.

var-const updated this revision to Diff 421412.Apr 7 2022, 9:28 PM
var-const marked 7 inline comments as done.

Address feedback, try to fix GCC again.

var-const added inline comments.Apr 8 2022, 1:17 PM
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 updated this revision to Diff 421664.Apr 8 2022, 6:38 PM

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

tcanens added inline comments.Apr 10 2022, 7:32 AM
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 added inline comments.Apr 11 2022, 8:11 AM
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 added inline comments.Apr 11 2022, 8:49 AM
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 updated this revision to Diff 422034.Apr 11 2022, 1:59 PM

Rebase on main, fix the CI.

var-const updated this revision to Diff 422072.Apr 11 2022, 4:35 PM

More GCC workarounds for access control.

var-const updated this revision to Diff 422155.Apr 12 2022, 2:52 AM

Fix the CI.

var-const updated this revision to Diff 422165.Apr 12 2022, 3:38 AM

Rebase on main.

var-const updated this revision to Diff 422171.Apr 12 2022, 4:07 AM

Undo accidental change.

var-const added inline comments.Apr 12 2022, 4:10 AM
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 added inline comments.Apr 12 2022, 4:58 AM
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 updated this revision to Diff 422183.Apr 12 2022, 5:02 AM

Fix the CI.

Try to fix the CI.

var-const updated this revision to Diff 422347.Apr 12 2022, 3:36 PM

Fix the CI?

var-const updated this revision to Diff 422372.Apr 12 2022, 5:42 PM

Fix the CI.

var-const updated this revision to Diff 422376.Apr 12 2022, 6:21 PM

Fix the CI?

var-const updated this revision to Diff 422390.Apr 12 2022, 9:03 PM

Fix the CI.

This revision was automatically updated to reflect the committed changes.