[libcxx-commits] [PATCH] D107671: [libcxx][ranges] Add `ranges::join_view`.

[Tim Song via Phabricator via libcxx-commits]

tcanens added inline comments.


================
Comment at: libcxx/include/__ranges/join_view.h:109
+            *__outer_,
+            __parent_->__inner_.__emplace_deref(__outer_));
+          __inner_ = ranges::begin(__inner);
----------------
ldionne wrote:
> tcanens wrote:
> > ldionne wrote:
> > > I don't understand why the spec allows modifying a member of the parent `join_view` from the iterator. That's very sneaky and can lead to e.g. race conditions if someone does:
> > > 
> > > ```
> > > join_view v = ...;
> > > auto it1 = v.begin();
> > > auto it2 = v.begin();
> > > 
> > > std::thread([=]{
> > >   ++it1; // calls satisfy() under the hood, which modifies `v`
> > > });
> > > 
> > > std::thread([=]{
> > >   ++it2; // calls satisfy() under the hood, which modifies `v`
> > > });
> > > 
> > > // sneaky race condition since you had a copy of different iterators in each thread
> > > ```
> > > 
> > > Basically, I think it's quite vexing that incrementing an iterator modifies the underlying view in this case, and I don't fully understand why that is even required. What use case does that enable? Is the criteria `is_reference_v<range_reference_t<V>>` meant to mean something like "the outer range creates its inner ranges on-the-fly and they are temporary objects"? I'm having trouble following here.
> > > 
> > > CC @tcanens in case you can share some insight here.
> > It's an input range for this case, so everything from the second `begin()` onwards is undefined.
> > 
> > Yes, this is the case where the inner ranges are created on the fly and we need to store the range //somewhere// while it is being iterated over, and the only reasonable place to store it is the view.
> Is there a reason why we don't use the criteria `input_range<V> && !forward_range<V>` to denote "it's only an input range and I can only do a single-pass on it" instead of `!is_reference_v<range_reference_t<V>>`?
> 
> Also, if everything from the second `begin()` onwards is undefined, then we can only ever have one iterator to that range. So would it be valid to store the temporary inner range in the iterator itself instead of storing it in the parent `join_view`?
`V`, the underlying range, can be anything up to random access (for instance, `iota(0, 42) | transform(to_string)`), but if it is a range of prvalue ranges, then `join_view<V>` can only ever be input.

It's not valid to store the inner range in the iterator itself without an indirection. Iterators are movable, `join_view`'s iterator must store an iterator into the inner range that is currently being iterated over, but moving a range can invalidate iterators into it.

IIRC Eric once said something along the lines of "views are lazy algorithms, and some algorithms have state". This is one of those stateful algorithms.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107671