[libcxx-commits] [PATCH] D136268: [libc++][ranges] implement `std::views::elements_view`
Konstantin Varlamov via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Mon Nov 28 17:56:48 PST 2022
var-const added a comment.
Sorry about the very slow review. Looks good with just a few nits/comments.
================
Comment at: libcxx/include/__ranges/elements_view.h:81
+ {
+ return __iterator<false>(ranges::begin(__base_));
+ }
----------------
var-const wrote:
> Optional: consider using `__iterator</*_Const=*/false>`. Personally, I find this trick of "annotating" the meaning of literals pretty helpful when reading code.
Please apply below as well (with `__sentinel`, mostly).
================
Comment at: libcxx/include/__ranges/elements_view.h:133
+
+ _LIBCPP_NO_UNIQUE_ADDRESS _View __base_ = _View();
+};
----------------
Does this also need a test?
================
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>>
----------------
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>>;
```
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.elements/adaptor.pass.cpp:138
+
+ // Test views::values | views::values
+ {
----------------
I'd suggest also testing e.g. `keys | values`, or any other case where the two adaptors use different indices.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.elements/base.pass.cpp:76
+ }
+ return true;
+}
----------------
Ultranit: please add a newline before this line.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.elements/ctor.view.pass.cpp:29
+static_assert(std::is_constructible_v<std::ranges::elements_view<View, 0>, View>);
+static_assert(!std::is_constructible_v<View, std::ranges::elements_view<View, 0>>);
+
----------------
This should probably be `std::is_convertible_v`.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.elements/general.pass.cpp:30
+ std::map{std::pair{"Lovelace"sv, 1815}, {"Turing"sv, 1912}, {"Babbage"sv, 1791}, {"Hamilton"sv, 1936}};
+ auto expectedYears = {1791, 1936, 1815, 1912};
+
----------------
Nit: I don't feel strongly about using `snake_case` or `camelCase` for local variables (I _personally_ use snake case when I can, but I don't think we have a hard rule on that), but it would be better to use one style consistently (currently `historical_figures` and `expectedYears` are inconsistent with each other).
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.elements/iterator/arithmetic.pass.cpp:48
+template <class BaseRange>
+using EleIter = std::ranges::iterator_t<std::ranges::elements_view<BaseRange, 0>>;
+
----------------
Nit: I think `ElemIter` would be a little better as a name (applies throughout).
================
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>>);
----------------
Question: is the empty comment to enforce a certain formatting?
================
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);
----------------
Question: why not use `std::same_as<> decltype(auto)` on the previous line instead?
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.elements/iterator/base.pass.cpp:82
+
+ using MoveOnlyEleIter =
+ std::ranges::iterator_t<std::ranges::elements_view<std::ranges::subrange<MoveOnlyIter, Sent>, 0>>;
----------------
Same nit: `s/Ele/Elem/`.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.elements/iterator/compare.pass.cpp:34
+ assert(iter1 < iter2);
+ assert(!(iter2 < iter1));
+ assert(iter1 <= iter1);
----------------
Nit: empty lines after each comparison operator would be pretty helpful.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.elements/iterator/compare.pass.cpp:111
+ auto it2 = ev.end();
+ assert(it1 != it2);
+
----------------
Optional: check that `it1 == it1` and `it2 == it2` as well? (Maybe even the same for the `!=` operator)
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.elements/iterator/compare.pass.cpp:131
+ using EleIter = decltype(it);
+ static_assert(!std::invocable<std::equal_to<>, EleIter, EleIter>);
+ inequalityOperatorsDoNotExistTest(it, it);
----------------
Perhaps also check `not_equal_to`, for completeness' sake?
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.elements/iterator/ctor.base.pass.cpp:11
+
+// constexpr explicit iterator(iterator_t<Base> current);;
+
----------------
Nit: extra semicolon.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.elements/iterator/ctor.default.pass.cpp:49
+ }
+ return true;
+}
----------------
Ultranit: please add a newline before this line.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.elements/iterator/decrement.pass.cpp:67
+ }
+ } else {
+ auto it = ev.begin();
----------------
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`.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.elements/iterator/decrement.pass.cpp:67
+ }
+ } else {
+ auto it = ev.begin();
----------------
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.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.elements/iterator/increment.pass.cpp:51
+ assert(base(result.base()) == &ts[0]);
+ } else {
+ auto it = ev.begin();
----------------
Optional: consider adding a blank line before this line.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.elements/range.concept.compile.pass.cpp:45
+
+// !view<V>
+static_assert(!HasElementsView<std::array<std::tuple<int>, 1>, 0>);
----------------
Optional: would it make sense to also `static_assert` these conditions? If you think it's overkill, feel free to disregard.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.elements/sentinel/minus.pass.cpp:99
+template <class T, class U>
+concept HasMinus = std::invocable<std::minus<>, const T&, const U&>;
+
----------------
Optional nit: would writing a constraint be slightly more readable? (Maybe not -- feel free to push back)
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.elements/size.pass.cpp:44
+ constexpr auto end() const { return iterator{buffer_ + size_}; }
+ constexpr std::size_t size() { return size_; }
+};
----------------
Very optional: I think theoretically it's also possible to have a `size() const` without `size()` (if you `= delete` the non-const `size`), but it's probably too convoluted to test.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.elements/types.h:36
+template <bool Simple>
+struct Common : TupleBufferView {
+ using TupleBufferView::TupleBufferView;
----------------
Ultranit: extra space.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.elements/types.h:90-92
+ using iterator_concept = std::random_access_iterator_tag;
+ using value_type = std::tuple<int>;
+ using difference_type = intptr_t;
----------------
Ultranit: the assignments are slightly unaligned.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D136268/new/
https://reviews.llvm.org/D136268
More information about the libcxx-commits
mailing list