[libcxx-commits] [PATCH] D107500: [libc++][ranges] Implement `lazy_split_view`.
Konstantin Varlamov via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Fri Mar 25 03:18:34 PDT 2022
var-const added inline comments.
================
Comment at: libcxx/include/__ranges/lazy_split_view.h:51
+ requires { typename __require_constant<remove_reference_t<_Range>::size()>; } &&
+ (remove_reference_t<_Range>::size() <= 1);
+
----------------
ldionne wrote:
> 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).
Done (in `range.lazy.split/constraints.compile.pass.cpp`). Great suggestion, thanks.
================
Comment at: libcxx/include/__ranges/lazy_split_view.h:59-60
+
+ _View __base_ = _View();
+ _Pattern __pattern_ = _Pattern();
+
----------------
ldionne wrote:
> 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.
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.
================
Comment at: libcxx/include/__ranges/lazy_split_view.h:210
+ if (__current() == __end) {
+ __trailing_empty_ = false;
+ return *this;
----------------
ldionne wrote:
> 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)?
The test case with a trailing separator (`"abc def "`) starts failing. `trailing_empty_` was added specifically for this case -- see [[ https://wg21.link/p2210 | P2210 "Superior String Splitting" ]] and [[ https://wg21.link/lwg3478 | LWG 3478 ]] that was fixed by it.
================
Comment at: libcxx/include/__ranges/lazy_split_view.h:234
+ }
+ break; // The pattern matched; skip it.
+ }
----------------
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.
================
Comment at: libcxx/include/__ranges/lazy_split_view.h:250
+ } else {
+ ++*this;
+ }
----------------
ldionne wrote:
> Do you test that this returns `void` on non-`forward_range`s?
Yes, it's tested in `range.lazy.split/range.lazy.split.outer/increment.pass.cpp`. Thanks for checking!
================
Comment at: libcxx/include/__ranges/lazy_split_view.h:329
+ } else {
+ ++*this;
+ }
----------------
ldionne wrote:
> Same comment about checking for `void` return here.
Yes, should be checked in `range.lazy.split/range.lazy.split.inner/increment.pass.cpp`.
================
Comment at: libcxx/include/__ranges/lazy_split_view.h:357
+ return true;
+ if (__pcur == __pend)
+ return __x.__incremented_;
----------------
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.
================
Comment at: libcxx/include/__ranges/lazy_split_view.h:360
+
+ do {
+ if (*__cur != *__pcur)
----------------
ldionne wrote:
> Do you have a test that would fail if `__x.__incremented` was always false (or true) for a non-tiny-range pattern?
Yes, `range.lazy.split/range.lazy.split.inner/increment.pass.cpp` and `general.pass.cpp` (the case with an empty separator) start failing.
================
Comment at: libcxx/include/__ranges/lazy_split_view.h:71-72
+
+ _View __base_ = _View();
+ _Pattern __pattern_ = _Pattern();
+
----------------
ldionne wrote:
> `[[no_unique_address]]`?
Sorry -- which data member does this apply to?
================
Comment at: libcxx/include/__ranges/lazy_split_view.h:305
+ friend constexpr bool operator==(const __inner_iterator& __x, default_sentinel_t) {
+ auto [__pcur, __pend] = ranges::subrange{__x.__i_.__parent_->__pattern_};
+ const auto __end = ranges::end(__x.__i_.__parent_->__base_);
----------------
var-const wrote:
> Note: since there is no check for null on `__parent`, trying to compare a default-constructed `__inner_iterator` would result in a null-pointer dereference. I wonder if this is intentional (the implementation is from the Standard).
Note: filed an LWG issue (will try not to forget to add the number here once it's available).
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/adaptor.pass.cpp:111
+
+ // Test that one can call std::views::filter with arbitrary stuff, as long as we
+ // don't try to actually complete the call by passing it a range.
----------------
ldionne wrote:
> Also consider searching in this patch in case there are other incorrect references.
This was the only one I could find, thanks for noticing!
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/base.pass.cpp:74
+ {
+ constexpr View str("abc def");
+ std::ranges::lazy_split_view<View, std::string_view> v(str, " ");
----------------
ldionne wrote:
> I don't think this should be `constexpr` (applies elsewhere too).
These are leftovers. I went through all the test files and I think I removed all of them now.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/base.pass.cpp:77
+
+ std::same_as<View> auto result = v.base();
+ assert(result == str);
----------------
ldionne wrote:
> 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.
Replaced all uses of `same_as.*auto` with `same_as.*decltype(auto)` within this patch's tests.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/ctad.compile.pass.cpp:49-52
+ // Note: CTAD from (InputRange, TinyForwardRange) doesn't work -- the deduction guide wraps the pattern in
+ // `views::all_t`, resulting in `views::owning_view<TinyForwardRange>`. That type would never satisfy `tiny-range`
+ // because `views::owning_view` contains a member function `size()` that shadows the static `size()` in
+ // `TinyForwardRange`.
----------------
Quuxplusone wrote:
> var-const wrote:
> > Just wanted to draw attention to this -- it seems like it could be an oversight.
> `owning_view::size()` is still constexpr, though... ooh, but it's not //static//, and `tiny-range` for some reason requires the `size` member function to be //static//. Gross. @tcanens, any thoughts?
Note: I filed an LWG issue about this, will try to remember to add the number here once it's available.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/general.pass.cpp:306
+ test_function_call("abc def", ' ', std::array{"abc", "def"});
+ // `wchar_t`.
+ test_function_call(L"abc def", L' ', std::array{L"abc", L"def"});
----------------
Mordante wrote:
> Since not all plaforms support `wchar_t` this needs to be guarded by `#ifndef TEST_HAS_NO_WIDE_CHARACTERS`.
Thanks!
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