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

Sat Apr 23 04:16:38 PDT 2022

huixie90 added inline comments.

================
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,
----------------
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.

================
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...> {
+
----------------
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

================
Comment at: libcxx/include/__ranges/zip_view.h:286
+    requires __zip_all_forward<_Const, _Views...>
+  {
+    auto __tmp = *this;
----------------
var-const wrote:
> huixie90 wrote:
> > var-const wrote:
> > > Nit: a few of these functions should be formatted so that the opening brace is on the same line as the function name.
> > it is done by clang-format. happy to format manually. 
> > 
> > but imo it would be great to come up with a clang-format file that everyone is sort of OK-ish with it. It would save everyone's  time
> I fully agree, being able to use `clang-format` would be great (though note that it's not fully straightforward on our code base).
cool. I will hand format for now

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/ctor.views.pass.cpp:27
+static_assert(std::constructible_from<std::ranges::zip_view<SimpleCommon>, SimpleCommon>);
+static_assert(!implicitly_constructible_from<std::ranges::zip_view<SimpleCommon>, SimpleCommon>);
+
----------------
var-const wrote:
> huixie90 wrote:
> > var-const wrote:
> > > Wouldn't `std::convertible_to` work?
> > My original thought was `convertible_to` doesn't work for multiple arguments but `zip_view` constructor can take multiple arguments. so i implemented a multi-args version.
> > But then I forgot to test the multi-args case.
> Ah, right (you might also want to check that the default constructor is `explicit`, though it starts going into the overkill territory).
The default constructor is not `explicit` and it is tested already

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/ctor.views.pass.cpp:39
+  constexpr const int* begin() const { return &moves; }
+  constexpr const int* end() const { return &moves + 1; }
+};
----------------
var-const wrote:
> huixie90 wrote:
> > var-const wrote:
> > > Seems like this could be UB? Consider making `moves` a one-element array.
> > I don't think this is UB. AFAIK, this is how `single_view` is implemented
> > 
> > https://eel.is/c++draft/range.single#view-5
> Thanks for pointing it out -- it made me find [[ https://stackoverflow.com/a/48322944 | this post ]] that quotes the relevant part of the Standard (updated to the current draft):
> 
> > When an expression `J` that has integral type is added to... an expression `P` of pointer type...
> >
> > <...>
> >
> > (4.2) Otherwise, if `P` points to an array element `i` of an array object `x` with `n` elements (`[dcl.array]`),70 the expressions `P + J` and `J + P`... point to the (possibly-hypothetical) array element  `i + j` of `x` if  `0 ≤ i+j ≤ n`...
> >
> ><...>
> >
> >70) As specified in `[basic.compound]`, an object that is not an array element is considered to belong to a single-element array for this purpose and a pointer past the last element of an array of `n` elements is considered to be equivalent to a pointer to a hypothetical array element `n` for this purpose.
thanks for the explanation

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/iterator/compare.pass.cpp:33
+  constexpr int& operator*() const { return *it_; }
+  constexpr int& operator[](difference_type n) const { return it_[n]; }
+  constexpr smaller_than_iterator& operator++() {
----------------
var-const wrote:
> Question: is defining `operator[]` necessary?
yes. this is one of the requirement of `random_access_iterator`

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.zip/iterator/ctor.other.pass.cpp:47
+
+    static_assert(!std::is_same_v<decltype(iter1), decltype(iter2)>);
+
----------------
var-const wrote:
> Is it possible to also add a `static_assert` to show that the `convertible_to` constraint is not satisfied in this case? (Only if it's straightforward, it's basically to "test the test" and to make it a little more obvious what exactly it's testing)
yes good idea. added `static_assert` right after the declaration of `ConstIterIncompatibleView`

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122806