[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:
> 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107671