[libcxx-commits] [PATCH] D122806: [libc++] add zip_view and views::zip for C++23

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added a comment.
This revision is now accepted and ready to land.

I looked around and it seemed like this had a thorough review already. I just replied to the comments where I was @'ed.

Thanks to everyone involved for the work and especially @huixie90 for authoring the patch. This LGTM with the last few missing comments addressed.



================
Comment at: libcxx/include/__ranges/zip_view.h:240
+template <bool _Const>
+class zip_view<_Views...>::__iterator : public __zip_view_iterator_category_base<_Const, _Views...> {
+
----------------
huixie90 wrote:
> var-const wrote:
> > huixie90 wrote:
> > > var-const wrote:
> > > > Is the following section implemented and tested?
> > > > ```
> > > > If the invocation of any non-const member function of `iterator` exits via an exception, the iterator acquires a singular value.
> > > > ```
> > > > (http://eel.is/c++draft/range.zip#iterator-3)
> > > AFAIK, doing anything with iterator of singular value would result in undefined behaviour (except for very few operations). one question is can we take advantage of the undefined behaviour and not do anything here?
> > > 
> > > One thing we could do is to value initialise all the underlying iterators. but what if underlying iterators are not default constructible?
> > Perhaps we should test that the iterator is reassignable/destroyable, but perhaps it's overkill. Let's check with Louis @ldionne.
> I asked around and people seem to agree on that that paragraph is to empower implementations not to do anything by given less guarantees that things should work. But let's wait until Louis is back
Sorry, I feel I'm missing some context, but it seems to me that a singular iterator should still be destroyable and assignable-to (unless there's something in the spec that contradicts that, but I don't think there is). If that's correct, then we should make sure that works and test it. Please LMK if what I'm saying doesn't make sense, since I'm commenting out of context.


================
Comment at: libcxx/include/__ranges/zip_view.h:429
+  friend constexpr void iter_swap(const __iterator& __l, const __iterator& __r) noexcept(
+      (noexcept(ranges::iter_swap(declval<const iterator_t<__maybe_const<_Const, _Views>>&>(),
+                                  declval<const iterator_t<__maybe_const<_Const, _Views>>&>())) &&
----------------
var-const wrote:
> huixie90 wrote:
> > var-const wrote:
> > > Hmm, is this equivalent to the Standard's wording?
> > > ```
> > > The exception specification is equivalent to the logical AND of the following expressions:
> > > 
> > > noexcept(ranges::iter_swap(std::get<i>(l.current_), std::get<i>(r.current_)))
> > > 
> > > for every integer  `0 ≤ i < sizeof...(Views)`.
> > > ```
> > I think so. In the standard, it basically says all the `iter_swap(std::get<i>(l.current_), std::get<i>(r.current_)` are `noexcept`. `std::get` itself is always noexcept, so the standard basically equivalent to all the  `ranges::iter_swap` are `noexcept`.
> > 
> > Here the implementation is checking the same thing
> Sounds reasonable, but I'd ask Louis @ldionne to double-check just in case.
Yeah, this sounds reasonable to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122806