[libcxx-commits] [PATCH] D107500: [libc++][ranges] Implement `lazy_split_view`.
Louis Dionne via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Tue Mar 22 12:17:31 PDT 2022
ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.
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.
================
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);
+
----------------
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).
================
Comment at: libcxx/include/__ranges/lazy_split_view.h:59-60
+
+ _View __base_ = _View();
+ _Pattern __pattern_ = _Pattern();
+
----------------
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.
================
Comment at: libcxx/include/__ranges/lazy_split_view.h:155
+ [[nodiscard]] _LIBCPP_HIDE_FROM_ABI
+ constexpr auto& __current() const noexcept {
+ if constexpr (forward_range<_View>) {
----------------
`const auto&`? This is mostly for the reader, since it doesn't change the actual meaning.
================
Comment at: libcxx/include/__ranges/lazy_split_view.h:210
+ if (__current() == __end) {
+ __trailing_empty_ = false;
+ return *this;
----------------
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)?
================
Comment at: libcxx/include/__ranges/lazy_split_view.h:215
+ const auto [__pbegin, __pend] = ranges::subrange{__parent_->__pattern_};
+ if (__pbegin == __pend) {
+ ++__current();
----------------
`// Empty pattern: split on every element in the input range`
================
Comment at: libcxx/include/__ranges/lazy_split_view.h:218
+
+ } else if constexpr (__tiny_range<_Pattern>) {
+ __current() = ranges::find(std::move(__current()), __end, *__pbegin);
----------------
`// One-element pattern: use ranges::find because we can`?
================
Comment at: libcxx/include/__ranges/lazy_split_view.h:221
+ if (__current() != __end) {
+ ++__current();
+ if (__current() == __end)
----------------
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`.
================
Comment at: libcxx/include/__ranges/lazy_split_view.h:226
+
+ } else {
+ do {
----------------
`// General case for n-element pattern`
================
Comment at: libcxx/include/__ranges/lazy_split_view.h:228
+ do {
+ const auto [__b, __p] = std::ranges::mismatch(__current(), __end, __pbegin, __pend);
+ if (__p == __pend) {
----------------
(perhaps elsewhere too)
================
Comment at: libcxx/include/__ranges/lazy_split_view.h:234
+ }
+ break; // The pattern matched; skip it.
+ }
----------------
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?
================
Comment at: libcxx/include/__ranges/lazy_split_view.h:250
+ } else {
+ ++*this;
+ }
----------------
Do you test that this returns `void` on non-`forward_range`s?
================
Comment at: libcxx/include/__ranges/lazy_split_view.h:329
+ } else {
+ ++*this;
+ }
----------------
Same comment about checking for `void` return here.
================
Comment at: libcxx/include/__ranges/lazy_split_view.h:357
+ return true;
+ if (__pcur == __pend)
+ return __x.__incremented_;
----------------
Do we have tests with an empty pattern that do not exercise the tiny-range code path?
================
Comment at: libcxx/include/__ranges/lazy_split_view.h:360
+
+ do {
+ if (*__cur != *__pcur)
----------------
Do you have a test that would fail if `__x.__incremented` was always false (or true) for a non-tiny-range pattern?
================
Comment at: libcxx/include/__ranges/lazy_split_view.h:71-72
+
+ _View __base_ = _View();
+ _Pattern __pattern_ = _Pattern();
+
----------------
`[[no_unique_address]]`?
================
Comment at: libcxx/include/__ranges/lazy_split_view.h:40
+
+#if !defined(_LIBCPP_HAS_NO_RANGES)
+
----------------
var-const wrote:
> Quuxplusone wrote:
> > `#if !defined(_LIBCPP_HAS_NO_CONCEPTS) && !defined(_LIBCPP_HAS_NO_INCOMPLETE_RANGES)`
> > (and likewise on the `#endif` of course)
> >
> I //think// that now that libc++ no longer supports older compilers, the `_LIBCPP_HAS_NO_CONCEPTS` is unnecessary; but it's a C++20 feature, so it seems like the guard should be `#if _LIBCPP_STD_VER > 17 && !defined(_LIBCPP_HAS_NO_RANGES)`.
The guard should be `#if _LIBCPP_STD_VER > 17 && !defined(_LIBCPP_HAS_NO_INCOMPLETE_RANGES)`.
================
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.
----------------
Also consider searching in this patch in case there are other incorrect references.
================
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, " ");
----------------
I don't think this should be `constexpr` (applies elsewhere too).
================
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);
----------------
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.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/general.pass.cpp:46-49
+ if (expected_it == expected.end()) return false;
+
+ if (SmallString(e) != *expected_it) return false;
+ ++expected_it;
----------------
Non-blocking suggestion. Initially I thought the `++expected_it` was tied to the `if` because of the indentation.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/general.pass.cpp:237
+ }
+
+ // Empty separator.
----------------
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)?
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/general.pass.cpp:316
+
+ // Non-character input.
+ {
----------------
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.
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