[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.

[ldionne]

Thanks a lot for working on this! This is a monster of a view. It generally looks really good, but I do have a few comments and questions.

libcxx/include/__ranges/lazy_split_view.h

41

The guard should be #if _LIBCPP_STD_VER > 17 && !defined(_LIBCPP_HAS_NO_INCOMPLETE_RANGES).

52

Can you please add a test with a TinyRange that has a non-constexpr size() static function? It should not satisfy tiny-range. Same for a size that is > 1 (if you don't have one already).

60–61

We could add [[no_unique_address]] here -- I don't foresee this making much of a difference except for a stateless view and pattern (e.g. ones that would generate stuff on the fly), but we don't lose anything.

72–73

[[no_unique_address]]?

156

const auto&? This is mostly for the reader, since it doesn't change the actual meaning.

211

I do not understand the purpose of __trailing_empty (comment for the spec, not for this specific implementation).

What tests start failing if you stop setting __trailing_empty_ to false or true below? Or if you set it to the wrong value (e.g. true here)?

216

// Empty pattern: split on every element in the input range

219

// One-element pattern: use ranges::find because we can?

222

Perhaps we could add a comment like // make sure we point to after the separator we just found or something along those lines -- I was initially surprised that we incremented again after a successful ranges::find.

227

// General case for n-element pattern

229

(perhaps elsewhere too)

235

I am wondering what happens if we do something like this: split("hellofoofooworld"sv, "foo"sv). I would assume we get {"hello"sv, ""sv, "world"sv}, however my understanding is that we'll get {"hello"sv, "fooworld"sv} with the current implementation. Can you confirm what happens, and make sure that it's tested?

251

Do you test that this returns void on non-forward_ranges?

330

Same comment about checking for void return here.

358

Do we have tests with an empty pattern that do not exercise the tiny-range code path?

361

Do you have a test that would fail if __x.__incremented was always false (or true) for a non-tiny-range pattern?

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

111 ↗

(On Diff #416350)

Also consider searching in this patch in case there are other incorrect references.

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

74 ↗

(On Diff #416350)

I don't think this should be constexpr (applies elsewhere too).

77 ↗

(On Diff #416350)

It would be better to test the result of functions with ASSERT_SAME_TYPE when it's impossible to test it with same_as, which is (almost) the case for functions that return references. Because if v.base() returned View const& instead of View (as it should), we wouldn't catch it right now.

As you just said in the live review, I think you could also switch to decltype(auto) instead to catch this.

I suspect this comment applies to other functions as well.

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

46–49 ↗

(On Diff #416350)

Non-blocking suggestion. Initially I thought the ++expected_it was tied to the if because of the indentation.

237 ↗

(On Diff #416350)

Can you please make sure you have tests for all the corner cases you showed me (the ones we want to take to the reflector)?

316 ↗

(On Diff #416350)

Can we also add a test case with *really* non-trivial "characters", i.e. splitting a sequence of maps or something like that. It's stretching it, but I just want to confirm that it works, since it's a "corner" case.

[var-const]

Address feedback, use sentinel_wrapper and outer-iterator::value_type to
remove unnecessary boilerplate in types.h.

libcxx/include/__ranges/lazy_split_view.h

52

Done (in range.lazy.split/constraints.compile.pass.cpp). Great suggestion, thanks.

60–61

Done. I also added a (libc++-specific) test for this, and it reminded me that I don't test other [[no_unique_address]] attributes, so I added tests for those as well. Please let me know if you think it's overkill.

72–73

Sorry -- which data member does this apply to?

211

The test case with a trailing separator ("abc def ") starts failing. trailing_empty_ was added specifically for this case -- see P2210 "Superior String Splitting" and LWG 3478 that was fixed by it.

235

It works correctly and is tested in general.pass.cpp:

// Several separators in a row.
{
  std::array expected = {"abc"sv, ""sv, ""sv, ""sv, "def"sv};
  test_one("abc    def", short_sep, expected);
  test_one("abc12121212def", long_sep, expected);
}

If I'm reading the code correctly, it will keep applying mismatch and advancing current until it matches the pattern, then move current to the element past the pattern. So iterations would look like:

1
hellofoofoobar
        ^
2
hellofoofoobar
           ^
3
hellofoofoobar
              ^

On the first iteration, it matches hello, skips the first foo and moves current to the next foo. On the second iteration, it doesn't match anything, skips the second foo and moves current to bar. On the last iteration, it matches bar until the end of the input range.

251

Yes, it's tested in range.lazy.split/range.lazy.split.outer/increment.pass.cpp. Thanks for checking!

306

Note: filed an LWG issue (will try not to forget to add the number here once it's available).

330

Yes, should be checked in range.lazy.split/range.lazy.split.inner/increment.pass.cpp.

358

Yes, the "Empty separator" test case from general.pass.cpp should cover it.

361

Yes, range.lazy.split/range.lazy.split.inner/increment.pass.cpp and general.pass.cpp (the case with an empty separator) start failing.

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

111 ↗

(On Diff #416350)

This was the only one I could find, thanks for noticing!

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

74 ↗

(On Diff #416350)

These are leftovers. I went through all the test files and I think I removed all of them now.

77 ↗

(On Diff #416350)

Replaced all uses of same_as.*auto with same_as.*decltype(auto) within this patch's tests.

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

49–52 ↗

(On Diff #413750)

Note: I filed an LWG issue about this, will try to remember to add the number here once it's available.

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

306 ↗

(On Diff #416350)

Thanks!

[ldionne]

I think this looks good to me once all my comments have been addressed! Thanks a lot for the extreme thoroughness of the tests, you're really raising the bar.

I'll have a last look once everything is addressed, but I think we should be really close to merging this now. Thanks!

libcxx/include/__ranges/lazy_split_view.h

72–73

I think this was a previous comment I never submitted before, please ignore.

235

Just walked through it with you, I understand what's going on. Thanks!

267

We should add a _LIBCPP_ASSERT(__x.__parent_) here, because otherwise we run into UB.

345

Same here, we should add _LIBCPP_ASSERT. For both of those, we can actually add a simple test that we are crashing, look for what we do in other assert.FOO.pass.cpp tests in the test suite.

358

I think we need to add tests that exercise the view with an input range that is a forward_range and a pattern that is a tiny-range to check that this optimization works correctly even when the input range is a forward_range.

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

40–43 ↗

(On Diff #418168)

IMO it makes more sense to only check the negative cases here, since the positive ones are (or should be) tested "at runtime" below. This comment may apply to other methods you're testing in the patch, too.

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

48 ↗

(On Diff #418168)

We are currently missing runtime tests for these methods -- we only instantiate them but never actually run them in this test. I would suggest something like:

std::ranges::lazy_split_view<V, P> v;
auto it = v.begin();
static_assert(std::is_same_v<decltype(it)::iterator_concept, std::forward_iterator_tag>);
// You probably want to also test this like you do right now:
static_assert(std::is_same_v<decltype(**it), const char&>);

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

3 ↗

(On Diff #418168)

Maybe this file should be named ctor.copy_move.pass.cpp?

12 ↗

(On Diff #418168)

You could say "Test the implicitly-generated copy and move constructors since lazy_split_view has non-trivial members".

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

31–35 ↗

(On Diff #418168)

I think you should use sentinel_wrapper here instead. We should not be augmenting our iterator archetypes for single tests -- when we need to, it's actually a sign that we're doing something wrong (either in the test, in the implementation/spec or in the archetype itself).

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

15 ↗

(On Diff #418168)

We should be testing that this isn't explicit by using an implicit construction.

32–33 ↗

(On Diff #418168)

We should run those tests instead.

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

19 ↗

(On Diff #418168)

We should be testing with an input_range/forward_range here too.

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

2 ↗

(On Diff #418168)

The same comments I made for begin() apply here.

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

1 ↗

(On Diff #418168)

I really like that you're testing the correctness of the view using a more general approach like this. I think we should also be testing the case where we have an input range and a tiny view (etc...) here, because they use a different branch of if constexpr in various cases, like the outer_iterator::operator++.

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

28–36 ↗

(On Diff #418168)

Run instead (I think you already do below)!

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

18–19 ↗

(On Diff #418168)

Run this!

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

18–24 ↗

(On Diff #418168)

Run this!

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

15 ↗

(On Diff #418168)

Missing <cassert>

59–61 ↗

(On Diff #418168)

You said: I'll remove this comment.

I say: I would keep the comment but perhaps just remove the part about it being surprising. I think it's useful as a description of what you are testing here.

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

67 ↗

(On Diff #418168)

We need a test for the _LIBCPP_ASSERT.

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

18 ↗

(On Diff #418168)

Missing <cassert> and <type_traits> and "test_macros.h" (and perhaps others)

40 ↗

(On Diff #418168)

One thing you could do is assert(&i2 == &i) to make sure that we are returning *this and not a reference to some other random iterator. You could do this in the tests for outer-iterator too.

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

81 ↗

(On Diff #418168)

I might have called this chunk instead, but this is non-blocking.

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

23 ↗

(On Diff #418168)

This looks like a leftover.

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

19–21 ↗

(On Diff #418168)

This shouldn't be necessary -- let's figure out why we need it, and fix the root cause.

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

38 ↗

(On Diff #418168)

I don't think this comment is useful in this test, especially if we add a test for this behavior.

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

19–20 ↗

(On Diff #418168)

Run this!

libcxx/test/std/ranges/range.adaptors/range.lazy.split/range.lazy.split.outer.value/view_interface.pass.cpp

14 ↗

(On Diff #418168)

Same comment I made for view_interface.

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

28 ↗

(On Diff #418168)

It would be nice to run these tests instead.

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

21 ↗

(On Diff #418168)

Run this!

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

21 ↗

(On Diff #418168)

Run this!

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

21 ↗

(On Diff #418168)

Would it make sense to make this a template? Or to use something like

auto check = []<typename View> {
  ...
};

check.operator()<SplitViewDiff>();
check.operator()<SplitViewInput>();

This may apply to other tests as well. I would not spend a ton of energy trying to remove duplication, but when it's easy and doesn't increase complexity, I think it's worth it. I'll let you balance this as you see fit.

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

80 ↗

(On Diff #418168)

We should add a death test for the _LIBCPP_ASSERT you'll add. You'll need to put it in another file because death tests don't work on Windows.

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

12 ↗

(On Diff #418168)

Can you add a comment saying that more corner cases are tested elsewhere in general.pass.cpp? Otherwise, someone looking at this test in isolation would be tempted to add a bunch of corner cases, but that would end up duplicating something that already exists elsewhere.

libcxx/test/std/ranges/range.adaptors/range.lazy.split/small_string.h

29–33 ↗

(On Diff #418168)

I don't think we want to do this -- I think we want to explicitly test the difference between string literals and char arrays, not hide it in the tests.

84 ↗

(On Diff #418168)

Nitpick, but it's a good habit when defining non-template functions in header files.

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

21 ↗

(On Diff #418168)

I would simply check that std::is_base_of<lazy_split_view, view_interface<...>> is true, otherwise we're double-testing view_interface. However, I would keep the runtime tests since they're already there and it's kind of nice to confirm that something basic like .front() works on a lazy_split_view.

[var-const]

Address feedback.

[var-const]

libcxx/include/__ranges/lazy_split_view.h

306

https://cplusplus.github.io/LWG/issue3686

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

40–43 ↗

(On Diff #418168)

In most other cases I fully agree, but here, I did that partially to verify that CanCallBase works correctly (e.g. that it doesn't just return false all the time) and partially to show which kinds of calls are and aren't valid. I'm absolutely open to remove if you still feel like it's overkill, but just wanted to provide a quick rationale.

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

15 ↗

(On Diff #418168)

Added test/support/is_explicit_ctr.h that provides a concept that should work with a constructor with any number of arguments (including a default constructor), and added the check to all constructor tests in this patch. Please let me know if you think it's overkill.

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

316 ↗

(On Diff #416350)

Added a vector of maps (since neither is constexpr, only checking at runtime).

1 ↗

(On Diff #418168)

Added the following:

// In addition to the `(ForwardView, ForwardView)` case, test the `(ForwardView, tiny-range)` and `(InputView,
// tiny-range)` cases (all of which have unique code paths).
if constexpr (std::is_same_v<std::remove_reference_t<Separator>, char>) {
  assert(test_function_call(CopyableView(input), ForwardTinyView(separator), expected));
  assert(test_with_piping(CopyableView(input), ForwardTinyView(separator), expected));

  assert(test_function_call(InputView(input), ForwardTinyView(separator), expected));
  assert(test_with_piping(InputView(input), ForwardTinyView(separator), expected));
}

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

81 ↗

(On Diff #418168)

Went with segment if you don't mind (for some reason, I associate chunk with raw bytes or similar).

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

19–21 ↗

(On Diff #418168)

Looks like it was a leftover. Thanks for noticing!

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

28 ↗

(On Diff #418168)

I removed the positive is_constructible test, but I think the rest of this is necessary (or at least useful) to verify the setup. Please let me know if you feel it's excessive.

libcxx/test/std/ranges/range.adaptors/range.lazy.split/small_string.h

29–33 ↗

(On Diff #418168)

Done. This led to changes in general.pass.cpp (which I think are ultimately an improvement).

[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

40–43 ↗

(On Diff #418168)

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

27 ↗

(On Diff #420695)

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

41–49 ↗

(On Diff #420695)

Why do you have effectively the same test twice?

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

25 ↗

(On Diff #420695)

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

29 ↗

(On Diff #420695)

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.

40–42 ↗

(On Diff #420695)

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

27–31 ↗

(On Diff #420695)

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

20 ↗

(On Diff #420695)

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

27 ↗

(On Diff #420695)

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

41–49 ↗

(On Diff #420695)

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

25 ↗

(On Diff #420695)

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

29 ↗

(On Diff #420695)

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

40–42 ↗

(On Diff #420695)

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

27–31 ↗

(On Diff #420695)

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

20 ↗

(On Diff #420695)

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

27 ↗

(On Diff #420695)

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

27 ↗

(On Diff #420695)

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

27 ↗

(On Diff #420695)

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

27 ↗

(On Diff #420695)

@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

27 ↗

(On Diff #420695)

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.

libcxx/include/CMakeLists.txt

set(files
   __ranges/dangling.h
   __ranges/data.h
   __ranges/drop_view.h
   __ranges/empty_view.h
   __ranges/empty.h
   __ranges/enable_borrowed_range.h
   __ranges/enable_view.h
   __ranges/iota_view.h
+  __ranges/lazy_split_view.h
   __ranges/non_propagating_cache.h
   __ranges/ref_view.h
   __ranges/single_view.h
   __ranges/size.h
   __ranges/subrange.h
   __ranges/take_view.h
   __ranges/transform_view.h
   __ranges/view_interface.h

libcxx/include/__ranges/lazy_split_view.h

+// -*- C++ -*-
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+#ifndef _LIBCPP___RANGES_LAZY_SPLIT_VIEW_H
+#define _LIBCPP___RANGES_LAZY_SPLIT_VIEW_H
+
+#include <__config>
+#include <__iterator/concepts.h>
+#include <__iterator/incrementable_traits.h>
+#include <__iterator/iterator_traits.h>
+#include <__ranges/copyable_box.h>
+#include <__ranges/single_view.h>
+#include <__ranges/enable_borrowed_range.h>
+#include <__ranges/view_interface.h>
+#include <algorithm>
+#include <concepts>
+#include <type_traits>
+
+#if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
+#pragma GCC system_header
+#endif
+
+_LIBCPP_PUSH_MACROS
+#include <__undef_macros>
+
+_LIBCPP_BEGIN_NAMESPACE_STD
+
+#if !defined(_LIBCPP_HAS_NO_RANGES)
+
+// REVIEW-NOTE: Will be its own patch. Not up for review.
+
+template<class _I1, class _I2, class _Range,
+         class _P1 = std::identity, class _P2 = std::identity>
+concept indirectly_comparable =
+    indirect_binary_predicate<_Range, projected<_I1, _P1>, projected<_I2, _P2>>;
+
+// END-REVIEW-NOTE
+
+namespace ranges {
+  template<auto> struct __require_constant;
+
+  template<class _Range>
+  concept __tiny_range =
+    sized_range<_Range> &&
+    requires { typename __require_constant<remove_reference_t<_Range>::size()>; } &&
+    (remove_reference_t<_Range>::size() <= 1);
+
+  template<input_range _View, forward_range _Pattern>
+    requires view<_View> && view<_Pattern> &&
+             indirectly_comparable<iterator_t<_View>, iterator_t<_Pattern>, ranges::equal_to> &&
+             (forward_range<_View> || __tiny_range<_Pattern>)
+  class lazy_split_view : public view_interface<lazy_split_view<_View, _Pattern>> {
+    template<bool> struct __inner_iterator;
+
+    // TODO: LWG issue: if there's no current iterator this is unimplementable as far as I can tell.
+    template<class>
+    struct __current_iter_base {};
+
+    template<forward_iterator _Iter>
+    struct __current_iter_base<_Iter> {
+        _Iter __current_ = _Iter();
+    };
+
+    template<bool _Const>
+    struct __outer_iterator
+      : public __current_iter_base<iterator_t<__maybe_const<_Const, _View>>> {
+
+    private:
+      using _Parent = __maybe_const<_Const, lazy_split_view>;
+      using _Base = __maybe_const<_Const, _View>;
+
+    public:
+      _Parent *__parent_ = nullptr;
+      bool __trailing_empty_ = false;
+
+      using iterator_concept = conditional_t<forward_range<_Base>, forward_iterator_tag, input_iterator_tag>;
+      using iterator_category = input_iterator_tag;
+      using difference_type = range_difference_t<_Base>;
+      struct value_type
+        : view_interface<value_type> {
+      private:
+        __outer_iterator __i_ = __outer_iterator();
+      public:
+        value_type() = default;
+        constexpr explicit value_type(__outer_iterator __i)
+          : __i_(_VSTD::move(__i)) {}
+
+        constexpr __inner_iterator<_Const> begin() const { return __inner_iterator<_Const>{__i_}; }
+        constexpr default_sentinel_t end() const { return default_sentinel; }
+      };
+
+      __outer_iterator() = default; // TODO: shouldn't this require that base iterator is default ctorable?
+
+      constexpr explicit __outer_iterator(_Parent& __parent)
+        requires (!forward_range<_Base>)
+        : __parent_(_VSTD::addressof(__parent)) {}
+
+      constexpr __outer_iterator(_Parent& __parent, iterator_t<_Base> __current)
+        requires forward_range<_Base>
+        : __parent_(_VSTD::addressof(__parent)), __current_(_VSTD::move(__current)) {}
+
+      constexpr __outer_iterator(__outer_iterator<!_Const> __i)
+        requires _Const && convertible_to<iterator_t<_View>, iterator_t<_Base>>
+        : __parent_(__i.__parent_), __current_(_VSTD::move(__i.__current_)) {}
+
+      constexpr value_type operator*() const { return value_type{*this}; }
+
+      constexpr __outer_iterator& operator++() {
+        const auto __end = ranges::end(__parent_->__base_);
+        // LWG editorial: this should have a trailing underscore.
+        if (__current_ == __end) {
+          __trailing_empty_ = false;
+          return *this;
+        }
+
+        const auto __srange = ranges::subrange{__parent_->__pattern_};
+        const auto __pbegin = __srange.begin();
+        const auto __pend = __srange.end();
+
+        if (__pbegin == __pend) {
+          ++__current_;
+        } else if (__tiny_range<_Pattern>) {
+          __current_ = _VSTD::find(_VSTD::move(__current_), __end, *__pbegin);
+          if (__current_ != __end) {
+            ++__current_;
+            if (__current_ == __end)
+              __trailing_empty_ = true;
+          }
+        } else {
+          do {
+            const auto [__b, __p] = _VSTD::mismatch(__current_, __end, __pbegin, __pend);
+            if (__p == __pend) {
+              __current_ = __b;
+              if (__current_ == __end)
+                __trailing_empty_ = true;
+              break;
+            }
+          } while (++__current_ != __end);
+        }
+        return *this;
+      }
+
+      constexpr decltype(auto) operator++(int) {
+        if constexpr (forward_range<_Base>) {
+          auto __tmp = *this;
+          ++*this;
+          return __tmp;
+        }
+        ++*this;
+      }
+
+      friend constexpr bool operator==(const __outer_iterator& __x, const __outer_iterator& __y)
+        requires forward_range<_Base>
+      {
+        return __x.__current_ == __y.__current_ && __x.__trailing_empty_ == __y.__trailing_empty_;
+      }
+
+      friend constexpr bool operator==(const __outer_iterator& __x, default_sentinel_t) {
+        return __x.__current_ == ranges::end(__x.__parent_->__base_) && !__x.__trailing_empty_;
+      }
+    };
+
+    template<bool _Const> struct __inner_iterator {
+    private:
+      using _Base = __maybe_const<_Const, _View>;
+      __outer_iterator<_Const> __i_ = __outer_iterator<_Const>();
+      bool __incremented_ = false;
+
+    public:
+      using iterator_Concept = typename __outer_iterator<_Const>::iterator_concept;
+      using iterator_category = _If<
+        derived_from<typename iterator_traits<iterator_t<_Base>>::iterator_category, forward_iterator_tag>,
+        forward_iterator_tag,
+        typename iterator_traits<iterator_t<_Base>>::iterator_category
+      >;
+
+      using value_type = range_value_t<_Base>;
+      using difference_type = range_difference_t<_Base>;
+
+      __inner_iterator() = default;
+
+      constexpr explicit __inner_iterator(__outer_iterator<_Const> __i)
+        : __i_(_VSTD::move(__i)) {}
+
+      constexpr const iterator_t<_Base>& base() const& { return __i_.__current_; }
+      constexpr iterator_t<_Base> base() && { return _VSTD::move(__i_.__current_); }
+
+      constexpr decltype(auto) operator*() const { return *__i_.__current_; }
+
+      constexpr __inner_iterator& operator++() {
+        __incremented_ = true;
+
+        if constexpr (!forward_range<_Base>) {
+          if constexpr (_Pattern::size() == 0) {
+            return *this;
+          }
+        }
+
+        ++__i_.__current_;
+        return *this;
+      }
+      constexpr decltype(auto) operator++(int) {
+        if constexpr (forward_range<_Base>) {
+          auto __tmp = *this;
+          ++*this;
+          return __tmp;
+        }
+
+        ++*this;
+      }
+
+      friend constexpr bool operator==(const __inner_iterator& __x, const __inner_iterator& __y)
+        requires forward_range<_Base>
+      {
+        return __x.__i_.__current_ == __y.__i_.__current_;
+      }
+
+      friend constexpr bool operator==(const __inner_iterator& __x, default_sentinel_t) {
+        const auto __srange = ranges::subrange{__x.__i_.__parent_->__pattern_};
+              auto __pcur = __srange.begin();
+        const auto __pend = __srange.end();
+        const auto __end = ranges::end(__x.__i_.__parent_->__base_);
+
+        auto __cur = __x.__i_.__current_;
+        if (__cur == __end) return true;
+        if (__pcur == __pend)  return __x.__incremented_;
+
+        if constexpr (__tiny_range<_Pattern>) {
+          return *__cur == *__pcur;
+        }
+
+        do {
+          if (*__cur != *__pcur) return false;
+          if (++__pcur == __pend) return true;
+        } while (++__cur != __end);
+
+        return false; // TODO: shouldn't this be true?
+      }
+
+      friend constexpr decltype(auto) iter_move(const __inner_iterator& __i)
+        noexcept(noexcept(ranges::iter_move(__i.__i_.__current_)))
+      {
+        return ranges::iter_move(__i.__i_.__current_);
+      }
+
+      friend constexpr void iter_swap(const __inner_iterator& __x, const __inner_iterator& __y)
+        noexcept(noexcept(ranges::iter_swap(__x.__i_.__current_, __y.__i_.__current_)))
+        requires indirectly_swappable<iterator_t<_Base>>
+      {
+        ranges::iter_swap(__x.__i_.__current_, __y.__i_.__current_);
+      }
+    };
+
+  public:
+    _View __base_ = _View();
+    _Pattern __pattern_ = _Pattern();
+    __non_propagating_cache<iterator_t<_View>> __current_; // TODO: only if not forward_range.
+
+    lazy_split_view()
+      requires default_initializable<_View> && default_initializable<_Pattern> = default;
+
+    constexpr lazy_split_view(_View __base, _Pattern __pattern)
+      : __base_(_VSTD::move(__base)), __pattern_(_VSTD::move(__pattern)) {}
+
+    template<input_range _Range>
+      requires constructible_from<_View, views::all_t<_Range>> &&
+               constructible_from<_Pattern, single_view<range_value_t<_Range>>>
+    constexpr lazy_split_view(_Range&& __r, range_value_t<_Range> __e)
+      : __base_(views::all(_VSTD::forward<_Range>(__r)))
+      , __pattern_(ranges::single_view(_VSTD::move(__e))) {}
+
+    constexpr _View base() const& requires copy_constructible<_View> { return __base_; }
+    constexpr _View base() && { return _VSTD::move(__base_); }
+
+    constexpr auto begin() {
+      if constexpr (forward_range<_View>)
+        return __outer_iterator<__simple_view<_View>>{*this, ranges::begin(__base_)};
+      else {
+        __current_.__set(ranges::begin(__base_));
+        return __outer_iterator<false>{*this};
+      }
+    }
+
+    constexpr auto begin() const requires forward_range<_View> && forward_range<const _View> {
+      return __outer_iterator<true>{*this, ranges::begin(__base_)};
+    }
+
+    constexpr auto end() requires forward_range<_View> && common_range<_View> {
+      return __outer_iterator<__simple_view<_View>>{*this, ranges::end(__base_)};
+    }
+
+    constexpr auto end() const {
+      if constexpr (forward_range<_View> && forward_range<const _View> && common_range<const _View>) {
+        return __outer_iterator<true>{*this, ranges::end(__base_)};
+      }
+      return default_sentinel;
+    }
+  };
+
+  template<class _Range, class _Pattern>
+  lazy_split_view(_Range&&, _Pattern&&) -> lazy_split_view<views::all_t<_Range>, views::all_t<_Pattern>>;
+
+  template<input_range _Range>
+  lazy_split_view(_Range&&, range_value_t<_Range>)
+    -> lazy_split_view<views::all_t<_Range>, single_view<range_value_t<_Range>>>;
+} // namespace ranges
+
+#endif // !defined(_LIBCPP_HAS_NO_RANGES)
+
+_LIBCPP_END_NAMESPACE_STD
+
+_LIBCPP_POP_MACROS
+
+#endif // _LIBCPP___RANGES_LAZY_SPLIT_VIEW_H

libcxx/include/ranges

 #include <__ranges/dangling.h>
 #include <__ranges/data.h>
 #include <__ranges/drop_view.h>
 #include <__ranges/empty.h>
 #include <__ranges/empty_view.h>
 #include <__ranges/enable_borrowed_range.h>
 #include <__ranges/enable_view.h>
 #include <__ranges/iota_view.h>
+#include <__ranges/lazy_split_view.h>
 #include <__ranges/ref_view.h>
 #include <__ranges/single_view.h>
 #include <__ranges/size.h>
 #include <__ranges/subrange.h>
 #include <__ranges/take_view.h>
 #include <__ranges/transform_view.h>
 #include <__ranges/view_interface.h>
 #include <compare>          // Required by the standard.