[libcxx-commits] [PATCH] D107500: [libc++][ranges] Implement `lazy_split_view`.
Louis Dionne via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Fri Mar 25 13:11:34 PDT 2022
ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.
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!
================
Comment at: libcxx/include/__ranges/lazy_split_view.h:71-72
+
+ _View __base_ = _View();
+ _Pattern __pattern_ = _Pattern();
+
----------------
var-const wrote:
> ldionne wrote:
> > `[[no_unique_address]]`?
> Sorry -- which data member does this apply to?
I think this was a previous comment I never submitted before, please ignore.
================
Comment at: libcxx/include/__ranges/lazy_split_view.h:234
+ }
+ break; // The pattern matched; skip it.
+ }
----------------
var-const wrote:
> ldionne wrote:
> > 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?
> 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.
Just walked through it with you, I understand what's going on. Thanks!
================
Comment at: libcxx/include/__ranges/lazy_split_view.h:357
+ return true;
+ if (__pcur == __pend)
+ return __x.__incremented_;
----------------
var-const wrote:
> ldionne wrote:
> > Do we have tests with an empty pattern that do not exercise the tiny-range code path?
> Yes, the "Empty separator" test case from `general.pass.cpp` should cover it.
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`.
================
Comment at: libcxx/include/__ranges/lazy_split_view.h:266
+ friend constexpr bool operator==(const __outer_iterator& __x, default_sentinel_t) {
+ return __x.__current() == ranges::end(__x.__parent_->__base_) && !__x.__trailing_empty_;
+ }
----------------
We should add a `_LIBCPP_ASSERT(__x.__parent_)` here, because otherwise we run into UB.
================
Comment at: libcxx/include/__ranges/lazy_split_view.h:344
+ _LIBCPP_HIDE_FROM_ABI
+ friend constexpr bool operator==(const __inner_iterator& __x, default_sentinel_t) {
+ auto [__pcur, __pend] = ranges::subrange{__x.__i_.__parent_->__pattern_};
----------------
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.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/base.pass.cpp:40-43
+static_assert( CanCallBase<std::ranges::lazy_split_view<CopyableView, ForwardView>&>);
+static_assert( CanCallBase<std::ranges::lazy_split_view<CopyableView, ForwardView> const &>);
+static_assert( CanCallBase<std::ranges::lazy_split_view<CopyableView, ForwardView>&&>);
+static_assert( CanCallBase<std::ranges::lazy_split_view<CopyableView, ForwardView> const &&>);
----------------
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.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/begin.pass.cpp:48
+
+ std::ranges::lazy_split_view<V, P> v;
+ static_assert(OuterIterConceptSameAs<decltype(v), std::forward_iterator_tag>);
----------------
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&>);
```
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/copy.pass.cpp:3
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
----------------
Maybe this file should be named `ctor.copy_move.pass.cpp`?
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/copy.pass.cpp:12
+
+// class std::ranges::lazy_split_view;
+
----------------
You could say "`Test the implicitly-generated copy and move constructors since lazy_split_view has non-trivial members`".
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/ctad.compile.pass.cpp:31-35
+struct InputRange {
+ cpp20_input_iterator<const char*> begin() const;
+ std::default_sentinel_t end() const;
+};
+bool operator==(const cpp20_input_iterator<const char*>&, std::default_sentinel_t);
----------------
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).
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/ctor.range.pass.cpp:15
+// constructible_from<Pattern, single_view<range_value_t<Range>>>
+// constexpr lazy_split_view(Range&& r, range_value_t<Range> e);
+
----------------
We should be testing that this isn't explicit by using an implicit construction.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/ctor.range.pass.cpp:32-33
+ using V = std::ranges::lazy_split_view<std::string_view, std::string_view>;
+ static_assert( std::is_constructible_v<V, std::string_view, char>);
+ static_assert( std::is_constructible_v<V, char(&)[5], char>);
+ struct Empty {};
----------------
We should run those tests instead.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/ctor.view.pass.cpp:19
+
+constexpr bool test() {
+ std::string_view str = "abc def";
----------------
We should be testing with an input_range/forward_range here too.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/end.pass.cpp:2
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
----------------
The same comments I made for `begin()` apply here.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/general.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
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++`.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/range.lazy.split.inner/base.pass.cpp:28-36
+static_assert( CanCallBase<InnerIterForward&, const BaseIterForward&>);
+static_assert( CanCallBase<InnerIterForward const &, const BaseIterForward&>);
+static_assert( CanCallBase<InnerIterForward &&, BaseIterForward>);
+static_assert( CanCallBase<InnerIterForward const &&, const BaseIterForward&>);
+
+static_assert( CanCallBase<InnerIterInput&, const BaseIterInput&>);
+static_assert( CanCallBase<InnerIterInput const &, const BaseIterInput&>);
----------------
Run instead (I think you already do below)!
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/range.lazy.split.inner/ctor.default.pass.cpp:18-19
+
+static_assert(std::is_default_constructible_v<InnerIterForward>);
+static_assert(std::is_default_constructible_v<InnerIterInput>);
+
----------------
Run this!
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/range.lazy.split.inner/ctor.outer_iterator.pass.cpp:18-24
+static_assert( std::is_constructible_v<InnerIterForward, OuterIterForward>);
+static_assert( std::is_constructible_v<InnerIterInput, OuterIterInput>);
+// Is only constructible if both the outer and the inner iterators have the same constness.
+static_assert( std::is_constructible_v<InnerIterConst, OuterIterConst>);
+// Note: this works because of an implicit conversion (`OuterIterNonConst` is converted to `OuterIterConst`).
+static_assert( std::is_constructible_v<InnerIterConst, OuterIterNonConst>);
+static_assert( std::is_constructible_v<InnerIterNonConst, OuterIterNonConst>);
----------------
Run this!
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/range.lazy.split.inner/deref.pass.cpp:15
+#include <ranges>
+
+#include "../types.h"
----------------
Missing `<cassert>`
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/range.lazy.split.inner/deref.pass.cpp:59-61
+ // Note: this can be surprising. When the underlying range is an input range, `current` is stored in the
+ // `lazy_split_view` itself and shared between `inner-iterator`s. Consequently, incrementing one iterator
+ // effectively increments all of them.
----------------
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.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/range.lazy.split.inner/equal.pass.cpp:67
+ }
+
+ return true;
----------------
We need a test for the `_LIBCPP_ASSERT`.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/range.lazy.split.inner/increment.pass.cpp:18
+
+#include "../types.h"
+
----------------
Missing `<cassert>` and `<type_traits>` and `"test_macros.h"` (and perhaps others)
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/range.lazy.split.inner/increment.pass.cpp:40
+
+ decltype(auto) i2 = ++i;
+ static_assert(std::is_lvalue_reference_v<decltype(i2)>);
----------------
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.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/range.lazy.split.inner/iter_move.pass.cpp:81
+ SplitViewForward v("abc def", " ");
+ auto val = *v.begin();
+
----------------
I might have called this `chunk` instead, but this is non-blocking.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/range.lazy.split.inner/iter_swap.pass.cpp:23
+
+#include <iostream>
+
----------------
This looks like a leftover.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/range.lazy.split.outer.value/begin.pass.cpp:19-21
+constexpr bool operator==(const cpp20_input_iterator<char*>& lhs, const cpp20_input_iterator<char*>& rhs) {
+ return base(lhs) == base(rhs);
+}
----------------
This shouldn't be necessary -- let's figure out why we need it, and fix the root cause.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/range.lazy.split.outer.value/ctor.default.pass.cpp:38
+
+ // Note: attempting a check like `val.begin() == val.end()` leads to null pointer dereference.
+
----------------
I don't think this comment is useful in this test, especially if we add a test for this behavior.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/range.lazy.split.outer.value/ctor.iter.pass.cpp:19-20
+
+static_assert(std::is_constructible_v<ValueTypeForward, OuterIterForward>);
+static_assert(std::is_constructible_v<ValueTypeInput, OuterIterInput>);
+
----------------
Run this!
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/range.lazy.split.outer.value/view_interface.pass.cpp:14
+
+#include <ranges>
+
----------------
Same comment I made for `view_interface`.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/range.lazy.split.outer/ctor.copy.pass.cpp:28
+};
+static_assert( IsConstOuterIter<OuterIterConst>);
+
----------------
It would be nice to run these tests instead.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/range.lazy.split.outer/ctor.parent.pass.cpp:21
+
+static_assert( std::ranges::forward_range<SplitViewForward>);
+static_assert(!std::is_constructible_v<OuterIterForward, SplitViewForward&>);
----------------
Run this!
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/range.lazy.split.outer/ctor.parent_base.pass.cpp:21
+
+static_assert( std::ranges::forward_range<SplitViewForward>);
+static_assert( std::is_constructible_v<OuterIterForward, SplitViewForward&, std::ranges::iterator_t<ForwardView>>);
----------------
Run this!
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/range.lazy.split.outer/deref.pass.cpp:21
+
+constexpr bool test() {
+ // `View` is a forward range.
----------------
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.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/range.lazy.split.outer/equal.pass.cpp:80
+ }
+
+ return true;
----------------
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.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/range.lazy.split.outer/increment.pass.cpp:12
+
+// constexpr outer-iterator& outer-iterator::operator++();
+// constexpr decltype(auto) outer-iterator::operator++(int);
----------------
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.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/small_string.h:29-33
+ constexpr void omit_terminating_null() {
+ if (size_ > 0 && buffer_[size_ - 1] == '\0') {
+ --size_;
+ }
+ }
----------------
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.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/small_string.h:84
+
+constexpr SmallString operator "" _str(const char* ptr, size_t size) {
+ return SmallString(ptr, size);
----------------
Nitpick, but it's a good habit when defining non-template functions in header files.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/view_interface.pass.cpp:21
+
+using V = SplitViewForward;
+static_assert(std::ranges::forward_range<V>);
----------------
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`.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D107500/new/
https://reviews.llvm.org/D107500
More information about the libcxx-commits
mailing list