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

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const accepted this revision as: var-const.
var-const added a comment.

LGTM -- thanks a lot for working on this huge patch! I'll leave the final approval for @ldionne.



================
Comment at: libcxx/include/__ranges/zip_view.h:84
+template <class _Fun, class _Tuple1, class _Tuple2, size_t... _Indices>
+_LIBCPP_HIDE_FROM_ABI constexpr __tuple_or_pair<
+    invoke_result_t<_Fun&, typename tuple_element<_Indices, remove_cvref_t<_Tuple1>>::type,
----------------
huixie90 wrote:
> var-const wrote:
> > huixie90 wrote:
> > > var-const wrote:
> > > > Is specifying the complicated return type necessary here (i.e., could this function get away with just `auto`)?
> > > I guess I'd have to spell this type in the return statement (currently it is just `return {.....}`).
> > I mean, could you rely on CTAD? To be clear, it's just a question -- perhaps CTAD won't work here.
> Originally I relied on `tuple`'s CTAD, but that crashed gcc 11.2
> https://discord.com/channels/636084430946959380/636732894974312448/960574541254496337
> 
> Later I tried `make_tuple`. There are two issues.
> 1. It is inconsistent with the rest of the code, where in other places, two elements thing are stored in a pair. Although this function is internal only, but still... consistency is good.
> 
> 2. CTAD/`make_tuple` decays. In case the function returns references, I would get tuple of values instead of tuple of references. Again, this is not an issue for now, because the usages of this helper function only return values (booleans and integers at the moment). But again, if one day this is used with references, it would become a problem silently.
Oh, interesting, thanks for explaining. It's certainly not worth it to work around compiler bugs and such, the current state is fine.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/ctor.default.pass.cpp:39
+// The default constructor requires all underlying views to be default constructible.
+// It is implicitly required by the tuple's constructor. if any of the iterators are 
+// not default constructible, zip iterator's =default would be implicitly deleted.
----------------
Ultranit: capitalize.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122806