[libcxx-commits] [PATCH] D157193: [libc++][ranges] P2116R9: Implements `views::enumerate`
Konstantin Varlamov via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Tue Sep 12 12:00:49 PDT 2023
var-const added a comment.
Finished going through the tests -- some more feedback!
================
Comment at: libcxx/include/__ranges/enumerate_view.h:51
+template <view _View>
+ requires __range_with_movable_references<_View>
+class enumerate_view : public view_interface<enumerate_view<_View>> {
----------------
Are we testing this constraint? (sorry if I missed it)
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.enumerate/ctad.compile.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
Note: you might also need to add the new view to `libcxx/test/std/ranges/iterator_robust_against_adl.compile.pass.cpp`.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.enumerate/ctor.base.pass.cpp:61
+ {
+ EnumerateView view;
+
----------------
This seems to be testing the default constructor?
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.enumerate/ctor.base.pass.cpp:73
+ assert(ev == 1);
+ }
+ {
----------------
Nit: please add a blank line after this line to separate the test cases.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.enumerate/ctor.default.pass.cpp:35
+template <bool DefaultInitializable>
+struct DefaultInitializableView : std::ranges::view_base {
+ constexpr explicit DefaultInitializableView()
----------------
Nit: it seems like this view can be merged with `DefaultConstructibleView` and `NoDefaultView` can be removed altogether.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.enumerate/ctor.default.pass.cpp:44
+
+struct NoDefaultView : std::ranges::view_base {
+ NoDefaultView() = delete;
----------------
Nit: `s/NoDefaultView/NoDefaultCtrView/`?
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.enumerate/iterator/arithmetic.pass.cpp:78
+
+ RandomAccessRange r{&ts[0], &ts[0] + 5};
+ auto ev = r | std::views::enumerate;
----------------
Optional: I think it can be just `{ts, ts + 5}`.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.enumerate/iterator/arithmetic.pass.cpp:81
+
+ // operator+(x, n) operator+(n,x) and operator+=
+ {
----------------
Nit: add a comma?
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.enumerate/iterator/base.pass.cpp:15
+
+// constexpr sentinel_t<Base> base() const;
+
----------------
Is this the right synopsis?
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.enumerate/iterator/base.pass.cpp:53
+ EnumerateIterator it = view.begin();
+ std::same_as<Iterator> decltype(auto) result = std::move(it).base();
+ assert(base(base(result)) == array.begin());
----------------
Can we check that the iterator was moved?
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.enumerate/iterator/compare.three_way.pass.cpp:61
+ using Iterator = decltype(ev.begin());
+ static_assert(HasSpaceship<Iterator, Iterator>);
+
----------------
Nit: I don't think this check (and the helper concept) is necessary -- there are no constraints, and the compiler will complain when we try to call `operator<=>` anyway were it somehow not available.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.enumerate/iterator/compare.three_way.pass.cpp:78
+ using Subrange = std::ranges::subrange<Iterator>;
+ static_assert(!std::three_way_comparable<Iterator>);
+ using EnumerateView = std::ranges::enumerate_view<Subrange>;
----------------
Can we also have a similar check (but presumably evaluating to `true`) in the test case above?
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.enumerate/iterator/ctor.convert.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
We also need a `types.pass.cpp` or similar test file to check `iterator_concept`, `iterator_category` and other nested types of the iterator.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.enumerate/iterator/ctor.convert.pass.cpp:16
+// constexpr iterator(iterator<!Const> i)
+// requires Const && convertible_to<iterator_t<V>, iterator_t<Base>>;
+
----------------
Can we check the `convertible_to` constraint?
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.enumerate/iterator/ctor.convert.pass.cpp:50
+
+ auto [index, value] = *(++it);
+ assert(index == 1);
----------------
Hmm, this seems to test `operator++`?
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.enumerate/iterator/deref.pass.cpp:42
+
+ std::array array{0, 1, 2, 3, 4, 5, 6, 7, 8, 9};
+ EnumerateView view = make_enumerate_view(array.begin(), array.end());
----------------
Note: some tests rely on CTAD when initializing `std::array` and some pass the template arguments explicitly. Can this be made consistent across the patch? (I would slightly prefer relying on CTAD to avoid explicitly specifying the size, but either is fine)
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.enumerate/iterator/deref.pass.cpp:47
+ for (std::size_t index = 0; index < array.size(); ++index) {
+ std::same_as<Result> decltype(auto) result = *it;
+
----------------
Can we also check with a const iterator?
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.enumerate/iterator/equal.pass.cpp:15
+
+// friend constexpr bool operator==(const iterator& x, const iterator& y) noexcept;
+
----------------
Can we check the `noexcept` part?
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.enumerate/iterator/equal.pass.cpp:35
+
+ assert(it1 == it1);
+ assert(it1 != it2);
----------------
Can we also check reverse argument order for each case, and increment `it1` until it equals `it2` and check that?
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.enumerate/iterator/index.pass.cpp:43
+ for (std::size_t index = 0; index < array.size(); ++index) {
+ assert(std::cmp_equal(index, it.index()));
+
----------------
Let's also check the return type and `noexcept`, and make sure we can call `index()` on a const iterator.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.enumerate/iterator/iter_move.pass.cpp:17
+// noexcept(noexcept(ranges::iter_move(i.current_)) &&
+// is_nothrow_move_constructible_v<range_rvalue_reference_t<Base>>)
+
----------------
I don't think we're checking the `is_nothrow_move_constructible_v` constraint?
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.enumerate/iterator/subscript.pass.cpp:47
+
+ assert(std::get<0>(it[0]) == std::get<0>(*it));
+ assert(std::get<1>(it[0]) == std::get<1>(*it));
----------------
Can you add a quick comment to explain why we can't use `assert(it[0] == *it);` here instead of `std::get`?
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.enumerate/sentinel/base.pass.cpp:33
+
+ auto make_enumerate_view = [](auto begin, auto end) {
+ View view{Iterator(begin), Sentinel(Iterator(end))};
----------------
Nit: we only use this function once, consider inlining it.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.enumerate/sentinel/ctor.convert.pass.cpp:16
+// constexpr sentinel(sentinel<!Const> other)
+// requires Const && convertible_to<sentinel_t<V>, sentinel_t<Base>>;
+
----------------
Can we check the `convertible_to` constraint?
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.enumerate/sentinel/ctor.convert.pass.cpp:47
+ std::same_as<EnumerateSentinel> decltype(auto) s = view.end();
+ std::same_as<Sentinel> decltype(auto) sResult = s.base();
+ assert(base(base(sResult)) == array.end());
----------------
`s/sResult/sBase/`?
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.enumerate/sentinel/ctor.default.pass.cpp:15
+
+// iterator() requires default_initializable<iterator_t<Base>>
+
----------------
The synopsis is for `iterator`, not `sentinel`.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.enumerate/sentinel/ctor.default.pass.cpp:23
+
+template <class Iterator, class Sentinel = sentinel_wrapper<Iterator>>
+constexpr void test() {
----------------
IMO we should either test with other sentinel types or not make `Sentinel` a template parameter (throughout).
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.enumerate/sentinel/ctor.default.pass.cpp:32
+
+ assert(base(base(s1.base())) == base(base(s2.base())));
+
----------------
Question: why are so many calls to `base` necessary?
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.enumerate/sentinel/equal.pass.cpp:16
+// template<bool OtherConst>
+// requires sentinel_for<sentinel_t<Base>, iterator_t<maybe-const<OtherConst, V>>>
+// friend constexpr bool operator==(const iterator<OtherConst>& x, const sentinel& y);
----------------
Is it possible to also check the `OtherConst` case?
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.enumerate/sentinel/equal.pass.cpp:41
+
+ std::same_as<bool> decltype(auto) eqResult = (it == s);
+ assert(!eqResult);
----------------
Can we check `s == it` and `s != it` as well?
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.enumerate/sentinel/equal.pass.cpp:48
+constexpr bool tests() {
+ test<cpp17_input_iterator<int*>>();
+ test<cpp20_input_iterator<int*>>();
----------------
It should be possible to use `types::for_each` to cut down on the boilerplate a little bit (from `type_algorithms.h`). Applies to other similar parts of the patch as well.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.enumerate/sentinel/minus.pass.cpp:13
+
+// class enumerate_view
+
----------------
Nit: `s/enumerate_view/enumerate_view::sentinel/`?
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.enumerate/sentinel/minus.pass.cpp:85
+template <class T>
+struct BufferView : std::ranges::view_base {
+ T* buffer_;
----------------
Question: why does `BufferView` need to be a separate class? Can it be merged with `Range`?
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.enumerate/sentinel/minus.pass.cpp:93
+ template <std::size_t N>
+ constexpr BufferView(std::array<T, N>& arr) : buffer_(arr.data()), size_(N) {}
+};
----------------
Nit: missing include.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.enumerate/sentinel/minus.pass.cpp:130
+constexpr void testConstraints() {
+ // Base is not sized
+ {
----------------
Can we also `static_assert` that `Base` is not a `sized_range`? (and vice versa below)
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.enumerate/sentinel/minus.pass.cpp:133
+ using Base = Range<Iter, Sent>;
+ static_assert(!HasMinus<EnumerateSentinel<Base>, EnumerateIter<Base>>);
+ static_assert(!HasMinus<EnumerateIter<Base>, EnumerateSentinel<Base>>);
----------------
Optional nit: IMO it would be slightly easier to read if these were sorted, for example:
```
static_assert(!HasMinus<EnumerateIter<Base>, EnumerateSentinel<Base>>);
static_assert(!HasMinus<EnumerateIter<Base>, EnumerateConstSentinel<Base>>);
static_assert(!HasMinus<EnumerateConstIter<Base>, EnumerateSentinel<Base>>);
static_assert(!HasMinus<EnumerateConstIter<Base>, EnumerateConstSentinel<Base>>);
static_assert(!HasMinus<EnumerateSentinel<Base>, EnumerateIter<Base>>);
static_assert(!HasMinus<EnumerateSentinel<Base>, EnumerateConstIter<Base>>);
static_assert(!HasMinus<EnumerateConstSentinel<Base>, EnumerateIter<Base>>);
static_assert(!HasMinus<EnumerateConstSentinel<Base>, EnumerateConstIter<Base>>);
```
(below as well)
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.enumerate/size.pass.cpp:15
+
+// constexpr auto size() requires sized_range<V>
+// constexpr auto size() const requires sized_range<const V>
----------------
Nit: please add a semicolon at the end (so that it looks like a declaration)?
```
// constexpr auto size() requires sized_range<V>;
```
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.enumerate/size.pass.cpp:28
+static_assert(HasSize<std::ranges::enumerate_view<SizedRange>>);
+static_assert(HasSize<const std::ranges::enumerate_view<SizedRange>>);
+
----------------
Can we check with a (pathological) type that is sized if it's non-const but not sized if it's const?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D157193/new/
https://reviews.llvm.org/D157193
More information about the libcxx-commits
mailing list