[libcxx-commits] [PATCH] D107500: [NO MERGE][libcxx][ranges] Add `ranges::lazy_split_view`.

[Tim Song via Phabricator via libcxx-commits]

tcanens added inline comments.


================
Comment at: libcxx/include/__ranges/lazy_split_view.h:60
+
+    // TODO: LWG issue: if there's no current iterator this is unimplementable as far as I can tell.
+    template<class>
----------------
zoecarver wrote:
> This is the main problem. It seems like there are a few places where the standard expects that `_View` is a forward_view that is default constructible. That requirement is never explicitly stated, though.
> 
> @CaseyCarter @cjdb do you have any insight into this? 
The presentation is certainly hilariously prone to confusion: https://eel.is/c++draft/range.lazy.split.outer#1, note the distinction between //`current`// and //`current_`//. 

I think that's the source of the confusion given your "trailing underscore" comment below.


================
Comment at: libcxx/include/__ranges/lazy_split_view.h:97
+
+      __outer_iterator() = default; // TODO: shouldn't this require that base iterator is default ctorable?
+
----------------
If the base iterator is at least forward, it already must be `regular`; if the base iterator is input, then we don't actually store it in the iterator, so this is still always default constructible.


================
Comment at: libcxx/include/__ranges/lazy_split_view.h:127
+          ++__current_;
+        } else if (__tiny_range<_Pattern>) {
+          __current_ = _VSTD::find(_VSTD::move(__current_), __end, *__pbegin);
----------------
The `constexpr` is not optional here.


================
Comment at: libcxx/include/__ranges/lazy_split_view.h:229
+
+        auto __cur = __x.__i_.__current_;
+        if (__cur == __end) return true;
----------------
This doesn't work for move-only input iterators (that's why the `if constexpr` in the spec defines `cur` differently in the two branches).


================
Comment at: libcxx/include/__ranges/lazy_split_view.h:242
+
+        return false; // TODO: shouldn't this be true?
+      }
----------------
This case is "we are pointing to a proper prefix of the pattern at the end of the source range". That doesn't match the pattern, so the range isn't yet exhausted. `false` is correct.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D107500/new/

https://reviews.llvm.org/D107500