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

Tim Song via Phabricator via libcxx-commits libcxx-commits at lists.llvm.org
Mon Apr 11 08:49:06 PDT 2022

tcanens 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>
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.

  rG LLVM Github Monorepo



More information about the libcxx-commits mailing list