[libcxx-commits] [PATCH] D102037: [libcxx][views] Add drop_view.

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver added inline comments.


================
Comment at: libcxx/include/__ranges/drop_view.h:44
+    range_difference_t<_View> __count = 0;
+    iterator_t<_View> __cached_begin = ranges::begin(__base);
+
----------------
tcanens wrote:
> cjdb wrote:
> > tcanens wrote:
> > > tcanens wrote:
> > > > There's no requirement that `ranges::begin` on a default-constructed view is even well-defined (see: `ref_view`).
> > > > 
> > > > Additionally, when a `drop_view` is copied, the cache must be invalidated in the copy.
> > > ...and when a `drop_view` is moved-from, the cache must be invalidated in the original.
> > @tcanens is this a non-propagating cache?
> Yep. This is the original use case of //`non-propagating-cache`// in range-v3.
Where in the standard does it say this is a non-propagating cache? Or is that what your paper does?


================
Comment at: libcxx/include/__ranges/drop_view.h:61
+      auto __tmp = ranges::begin(__base);
+      if constexpr (forward_range<_View>) {
+        if (__cached_begin != __tmp || __count == 0)
----------------
ldionne wrote:
> I think it would be more natural to use
> 
> ```
> if (!input_or_output_iterator<iterator_t<_View>>) {
>  // cache here
> }
> ```
Unfortunately, I just realized that all iterators model input or output iterator, so I think we'll have to stick with `forward_range`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102037