[libcxx-commits] [PATCH] D107500: [libc++][ranges] Implement `lazy_split_view`.

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added inline comments.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/begin.pass.cpp:27
+
+constexpr bool test() {
+  // non-const: forward_range<View> && simple-view<View> -> outer-iterator<Const = true>
----------------
var-const wrote:
> tcanens wrote:
> > ldionne wrote:
> > > tcanens wrote:
> > > > var-const wrote:
> > > > > philnik wrote:
> > > > > > I looks like you aren't checking that `begin()` caches the result. Could you add a test for that?
> > > > > 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?
> > > > No. Caching input iterators doesn't make sense, and `begin` doesn't perform any non-constant time operation that requires caching anyway.
> > > 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.
> > 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.
> @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(...)`.
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107500