[libcxx-commits] [PATCH] D136268: [libc++][ranges] implement `std::views::elements_view`

[Hui via Phabricator via libcxx-commits]

huixie90 added inline comments.


================
Comment at: libcxx/include/__tuple/make_tuple_types.h:65
+  template <size_t _Index>
+  using __element_type = std::conditional_t<_Index == 0, _Iter, _Sent>;
+  template <class _Tp, class _ApplyFn = __apply_cv_t<_Tp>>
----------------
var-const wrote:
> huixie90 wrote:
> > var-const wrote:
> > > Question: can you explain the idea here?
> > the spec defines `subrange` is also a `tuple-like`
> > https://eel.is/c++draft/tuple.like#concept:tuple-like
> > 
> > Before this patch, libc++ defines tuple-like to be only `pair`, `array` and `tuple`
> > https://github.com/llvm/llvm-project/blob/main/libcxx/include/__tuple/tuple_like.h
> > 
> > This patch makes `subrange` also a `tuple-like`. (see the diff in tuple_like.h)
> > 
> > The file in question is creating some meta functions to create a flat list of underlying types given a tuple-like type, and this function is used in the conversions between tuple-like types. One example would be `pair::pair(pair-like auto&&)`
> > 
> > You will see the few line above there are several specialisations.
> > 1. for `tuple<Ts...>` it just produces `Ts...`
> > 2. for `array<T, N>` it produces `T, T, T, T, ...`
> > 3. for `pair<First, Second>`, it produces `First, Second`
> > Now I am adding another one
> > 4. for `subrange<Iter, Sent, Kind>`, it produces `Iter, Sent` (note that `subrange` has a non-type template parameter at the 3rd place, so it won't match any of the existing specialisations.
> > 
> > 
> Thanks for the explanation! In that case, perhaps the variadic `__element_type` is unnecessary? Would something like this work?
> ```
> template <class _Tp, class _ApplyFn = __apply_cv_t<_Tp>>
> using __apply_quals = __tuple_types<
>     typename _ApplyFn::template __apply<_Iter>,
>     typename _ApplyFn::template __apply<_Sent>>;
> ```
Yes that looks much better. I will remove the `...`.
Thanks for the suggestion


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.elements/iterator/arithmetic.pass.cpp:52
+static_assert(std::ranges::random_access_range<RandomAccessRange>);
+static_assert(std::sized_sentinel_for<std::ranges::iterator_t<RandomAccessRange>, //
+                                      std::ranges::iterator_t<RandomAccessRange>>);
----------------
var-const wrote:
> Question: is the empty comment to enforce a certain formatting?
Yes. if you have strong feelings I can remove them and put
```
// clang-format off
// clang-format on
```
around them


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.elements/iterator/base.pass.cpp:53
+    decltype(auto) base = it.base();
+    static_assert(std::is_same_v<decltype(base), std::tuple<int>* const&>);
+    assert(base == &t);
----------------
var-const wrote:
> Question: why not use `std::same_as<> decltype(auto)` on the previous line instead?
as of the patch was written (about 2-3 months ago?), Some compilers in the CI which were based on older version of clang has a bug where `std::same_as<Foo&> decltype(auto) = ...` does not work if the return type is a reference


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.elements/iterator/decrement.pass.cpp:67
+    }
+  } else {
+    auto it = ev.begin();
----------------
var-const wrote:
> var-const wrote:
> > Optional: consider switching the order of branches (so that the check becomes `if (!std::bidirectional_iterator)`). The `else` branch is much shorter, so if it's close to the condition, it's easy to see both branches at the same time. In the current state it requires a bit of scrolling to find the `else`.
> Optional: consider adding a blank line before this line.
clang-format will remove that empty line


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136268