[libcxx-commits] [PATCH] D141699: [libc++][ranges] Implement P2474R2(`views::repeat`).
Konstantin Varlamov via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Wed May 31 23:37:29 PDT 2023
var-const requested changes to this revision.
var-const added a comment.
This revision now requires changes to proceed.
Sorry about the (very) slow review!
================
Comment at: libcxx/docs/FeatureTestMacroTable.rst:347
------------------------------------------------- -----------------
+ ``__cpp_lib_ranges_repeat`` *unimplemented*
+ ------------------------------------------------- -----------------
----------------
Shouldn't this be annotated with the corresponding date rather than being marked `*unimplemented*`?
================
Comment at: libcxx/include/__ranges/drop_view.h:278
+ class _Dist = range_difference_t<_Range>>
+ requires (!__is_empty_view<_RawRange> &&
+ __is_repeat_specialization<_RawRange> &&
----------------
Question: is `__is_empty_view` necessary here?
================
Comment at: libcxx/include/__ranges/drop_view.h:282
+ _LIBCPP_NODISCARD_EXT _LIBCPP_HIDE_FROM_ABI constexpr auto operator()(_Range&& __range, _Np&& __n) const noexcept(
+ noexcept(repeat_view(*ranges::begin(__range),
+ ranges::distance(__range) - std::min<_Dist>(ranges::distance(__range), std::forward<_Np>(__n)))))
----------------
Nit: in existing files, we try to format the three expressions (the actual return expression and its two duplicates inside the `noexcept` and the `decltype`) so that all the three are aligned, making it easier to quickly check that they are exactly the same. Can you format these similarly? Feel free to disable clang-format for those lines if it gets in the way.
================
Comment at: libcxx/include/__ranges/drop_view.h:284
+ ranges::distance(__range) - std::min<_Dist>(ranges::distance(__range), std::forward<_Np>(__n)))))
+ -> decltype(repeat_view(*ranges::begin(__range),
+ ranges::distance(__range) -
----------------
In the standard, it uses `views::repeat` rather than `repeat_view`, is there a reason not to do the same here?
================
Comment at: libcxx/include/__ranges/drop_view.h:284
+ ranges::distance(__range) - std::min<_Dist>(ranges::distance(__range), std::forward<_Np>(__n)))))
+ -> decltype(repeat_view(*ranges::begin(__range),
+ ranges::distance(__range) -
----------------
var-const wrote:
> In the standard, it uses `views::repeat` rather than `repeat_view`, is there a reason not to do the same here?
Question: the standard instead uses `__range.value_`, is using `begin` instead deliberate?
================
Comment at: libcxx/include/__ranges/repeat_view.h:43
+
+template <copy_constructible _Tp, semiregular _Bound = unreachable_sentinel_t>
+ requires(is_object_v<_Tp> && same_as<_Tp, remove_cv_t<_Tp>> &&
----------------
Hmm, it's `move_constructible` in the latest draft of the Standard. Is landing https://reviews.llvm.org/D151629 a prerequisite to do that?
================
Comment at: libcxx/include/__ranges/repeat_view.h:45
+ requires(is_object_v<_Tp> && same_as<_Tp, remove_cv_t<_Tp>> &&
+ (__integer_like<_Bound> || same_as<_Bound, unreachable_sentinel_t>))
+class repeat_view : public view_interface<repeat_view<_Tp, _Bound>> {
----------------
The latest draft of the Standard uses `integer-like-with-usable-difference-type` which seems to have a couple more conditions. If this is deliberate, can you please add a comment with a TODO?
================
Comment at: libcxx/include/__ranges/repeat_view.h:45
+ requires(is_object_v<_Tp> && same_as<_Tp, remove_cv_t<_Tp>> &&
+ (__integer_like<_Bound> || same_as<_Bound, unreachable_sentinel_t>))
+class repeat_view : public view_interface<repeat_view<_Tp, _Bound>> {
----------------
var-const wrote:
> The latest draft of the Standard uses `integer-like-with-usable-difference-type` which seems to have a couple more conditions. If this is deliberate, can you please add a comment with a TODO?
Question: can `_Bound` be a user-defined type?
================
Comment at: libcxx/include/__ranges/repeat_view.h:54
+
+ _LIBCPP_HIDE_FROM_ABI constexpr explicit repeat_view(const _Tp& __value, _Bound __bound_sentinel = _Bound())
+ : __value_(in_place, __value), __bound_(__bound_sentinel) {
----------------
`requires copy_constructible<T>`?
================
Comment at: libcxx/include/__ranges/repeat_view.h:57
+ if constexpr (!same_as<_Bound, unreachable_sentinel_t>)
+ _LIBCPP_ASSERT(__bound_ >= 0, "The value of bound must be greater equal than 0");
+ }
----------------
`must be greater than or equal to 0`
================
Comment at: libcxx/include/__ranges/repeat_view.h:68
+ requires(constructible_from<_Tp, _TpArgs...> && constructible_from<_Bound, _BoundArgs...>)
+ _LIBCPP_HIDE_FROM_ABI constexpr explicit repeat_view(
+ piecewise_construct_t, tuple<_TpArgs...> __value_args, tuple<_BoundArgs...> __bound_args = tuple<>{})
----------------
Perhaps we could add an assertion for the `The behavior is undefined if Bound is not unreachable_sentinel_t and bound_ is negative` clause?
================
Comment at: libcxx/include/__ranges/repeat_view.h:109
+ if constexpr (!same_as<_Bound, unreachable_sentinel_t>)
+ _LIBCPP_ASSERT(__current_ >= 0, "The value of bound must be greater equal than 0");
+ }
----------------
Question: this is already asserted in the constructor of the view. What is the situation where it needs to be asserted again when creating the iterator?
================
Comment at: libcxx/include/__ranges/repeat_view.h:133
+
+ _LIBCPP_HIDE_FROM_ABI constexpr __iterator& operator--() {
+ --__current_;
----------------
Perhaps assert on `Preconditions: If Bound is not unreachable_sentinel_t, current_ > 0`?
================
Comment at: libcxx/test/libcxx/ranges/range.factories/range.repeat.view/ctor.value.bound.verify.cpp:20
+int main(int, char**) {
+ { TEST_LIBCPP_ASSERT_FAILURE(std::ranges::repeat_view(0, -1), "The value of bound must be greater equal than 0"); }
+
----------------
philnik wrote:
> Why do you have an extra scope around this?
We need to check both constructors (and the iterator's constructor if we keep the assertion there).
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.drop/adaptor.pass.cpp:224
+#if TEST_STD_VER >= 23
+ // `views::drop(repeat_view, n)` returns an `repeat_view` when repeat_view models sized_range.
+ {
----------------
Ultranit: `s/returns an/return a/`.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.drop/adaptor.pass.cpp:224
+#if TEST_STD_VER >= 23
+ // `views::drop(repeat_view, n)` returns an `repeat_view` when repeat_view models sized_range.
+ {
----------------
var-const wrote:
> Ultranit: `s/returns an/return a/`.
Ultranit: can you also put backticks around `repeat_view` and `sized_range` (here and in the similar comment below)? Otherwise it's a little inconsistent.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.drop/adaptor.pass.cpp:224
+#if TEST_STD_VER >= 23
+ // `views::drop(repeat_view, n)` returns an `repeat_view` when repeat_view models sized_range.
+ {
----------------
var-const wrote:
> var-const wrote:
> > Ultranit: `s/returns an/return a/`.
> Ultranit: can you also put backticks around `repeat_view` and `sized_range` (here and in the similar comment below)? Otherwise it's a little inconsistent.
Can you also add a `static_assert` to show that the view models `sized_range` (and similarly doesn't model `sized_range` below)?
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.drop/adaptor.pass.cpp:233
+
+ // `views::drop(repeat_view, n)` returns an `repeat_view` when repeat_view not models sized_range.
+ {
----------------
Ultranit: `s/not models/doesn't model/`.
================
Comment at: libcxx/test/std/ranges/range.factories/range.repeat.view/begin.pass.cpp:18
+constexpr bool test() {
+ {
+ std::ranges::repeat_view<int> rv(0);
----------------
Can you add a short comment to each test case to briefly explain what it does? It makes tests much easier to read.
================
Comment at: libcxx/test/std/ranges/range.factories/range.repeat.view/begin.pass.cpp:44
+ {
+ const std::ranges::repeat_view<int, long long> rv(0, 1024);
+ std::same_as<std::ranges::iterator_t<decltype(rv)>> decltype(auto) iter = rv.begin();
----------------
Nit: I'd suggest `s/1024/10/` here to make it more obvious that this test case checks for the same thing as the previous one but for a const `repeat_view`.
================
Comment at: libcxx/test/std/ranges/range.factories/range.repeat.view/begin.pass.cpp:54
+ {
+ const std::ranges::repeat_view<int, unsigned long> rv(42, 33);
+ std::same_as<std::ranges::iterator_t<decltype(rv)>> decltype(auto) iter = rv.begin();
----------------
Same comment re. `s/33/20/` as above.
================
Comment at: libcxx/test/std/ranges/range.factories/range.repeat.view/ctad.compile.pass.cpp:15
+#include <ranges>
+#include <cassert>
+#include <concepts>
----------------
Nit: `<cassert>` can be removed.
================
Comment at: libcxx/test/std/ranges/range.factories/range.repeat.view/ctad.compile.pass.cpp:21
+// clang-format off
+static_assert(std::same_as<decltype(std::ranges::repeat_view(Empty())), std::ranges::repeat_view<Empty> >);
+static_assert(std::same_as<decltype(std::ranges::repeat_view(std::declval<Empty&>())), std::ranges::repeat_view<Empty> >);
----------------
Ultranit: if the space between the two closing angle brackets is deliberate, then perhaps also add a space after the corresponding opening angle bracket?
================
Comment at: libcxx/test/std/ranges/range.factories/range.repeat.view/ctad.compile.pass.cpp:25
+static_assert(std::same_as<decltype(std::ranges::repeat_view(Empty(), 1)), std::ranges::repeat_view<Empty, int> >);
+static_assert(std::same_as<decltype(std::ranges::repeat_view(std::declval<Empty&>(), 1U)), std::ranges::repeat_view<Empty, unsigned> >);
+static_assert(std::same_as<decltype(std::ranges::repeat_view(std::declval<Empty&&>(), 1UL)), std::ranges::repeat_view<Empty, unsigned long> >);
----------------
Nit: I'd just use `Empty()` for this line and the line below -- we already checked the references deduce to a non-reference type above, and these 3 lines seem focused on how the bound is deduced.
================
Comment at: libcxx/test/std/ranges/range.factories/range.repeat.view/ctad.compile.pass.cpp:29
+
+int main(int, char**) { return 0; }
----------------
philnik wrote:
> You don't need a `main` in a `.compile.pass.cpp` test.
Nit: this `main` is unnecessary.
================
Comment at: libcxx/test/std/ranges/range.factories/range.repeat.view/ctor.default.pass.cpp:18
+struct DefaultInt42 {
+ int __value_ = 42;
+};
----------------
These preceding underscores are unnecessary -- we only use them within the library to make sure the names of our variables/functions/etc. cannot clash with user-defined macros. It also creates an impression that the tests below are accessing some internal state.
(honestly, I'd even remove the trailing underscore but that's a matter of taste. Personally, I only use it for private variables)
================
Comment at: libcxx/test/std/ranges/range.factories/range.repeat.view/ctor.default.pass.cpp:23
+ int __value_;
+ Int(int v) : __value_(v) {}
+};
----------------
Optional: since this struct is only used to do a compile-time check, you could remove the member variable and just have the constructor defined as `Int(int) {}`.
================
Comment at: libcxx/test/std/ranges/range.factories/range.repeat.view/ctor.piecewise.pass.cpp:80
+ std::tuple<int>>);
+static_assert(!std::constructible_from<std::ranges::repeat_view<A>,
+ std::piecewise_construct_t,
----------------
Can we also test that the view is not constructible when given a tuple that can be used to construct `A` and a tuple that cannot be used to construct the bound?
================
Comment at: libcxx/test/std/ranges/range.factories/range.repeat.view/ctor.value.bound.pass.cpp:38
+
+ // Move && unbound && specific argument
+ {
----------------
Nit: consider replacing `specific` with `user-provided`, `explicitly provided` or similar.
================
Comment at: libcxx/test/std/ranges/range.factories/range.repeat.view/ctor.value.bound.pass.cpp:58
+ {
+ const Empty e;
+ std::ranges::repeat_view<Empty> rv(e);
----------------
Nit: this `e` is const but similar ones below aren't, can this be consistent?
================
Comment at: libcxx/test/std/ranges/range.factories/range.repeat.view/end.pass.cpp:11
+
+// constexpr auto end() const;
+// constexpr iterator end() const requires same_as<W, Bound>;
----------------
Nit: I think this should return `unreachable_sentinel_t` and be `noexcept`.
================
Comment at: libcxx/test/std/ranges/range.factories/range.repeat.view/end.pass.cpp:17
+#include <concepts>
+#include <utility>
+
----------------
Nit: `<utility>` seems unused, but `unreachable_sentinel_t` requires `<iterator>`.
================
Comment at: libcxx/test/std/ranges/range.factories/range.repeat.view/end.pass.cpp:30
+ {
+ std::ranges::repeat_view<int> rv(0);
+ assert(std::ranges::next(rv.begin(), 10) != rv.end());
----------------
Question: do we have a test where we iterate over a whole bounded view, i.e. go from `begin` to `end` and make sure the same value is repeated?
================
Comment at: libcxx/test/std/ranges/range.factories/range.repeat.view/iterator/compare.pass.cpp:18
+constexpr bool test() {
+ {
+ using R = std::ranges::repeat_view<int>;
----------------
Can you add comments to each test case?
================
Comment at: libcxx/test/std/ranges/range.factories/range.repeat.view/iterator/compare.pass.cpp:32
+
+ assert((iter1 <=> iter2) == std::strong_ordering::less);
+ assert((iter1 <=> iter1) == std::strong_ordering::equal);
----------------
I think we should also check the generated comparison operators (unless there's precedent for not doing so in existing tests).
================
Comment at: libcxx/test/std/ranges/range.factories/range.repeat.view/iterator/ctor.default.pass.cpp:17
+ using Iter = std::ranges::iterator_t<std::ranges::repeat_view<int>>;
+ static_assert(std::is_nothrow_default_constructible_v<Iter>);
+ Iter iter;
----------------
Question: is the default constructor guaranteed to be `nothrow`?
================
Comment at: libcxx/test/std/ranges/range.factories/range.repeat.view/iterator/ctor.default.pass.cpp:19
+ Iter iter;
+ (void)iter;
+
----------------
Nit: in new code, we prefer to use `[[maybe_unused]]` instead of a void cast.
================
Comment at: libcxx/test/std/ranges/range.factories/range.repeat.view/iterator/ctor.default.pass.cpp:19
+ Iter iter;
+ (void)iter;
+
----------------
var-const wrote:
> Nit: in new code, we prefer to use `[[maybe_unused]]` instead of a void cast.
Are there any properties of a default-constructed iterator that we could check? It it perhaps equal to `end()`?
================
Comment at: libcxx/test/std/ranges/range.factories/range.repeat.view/iterator/decrement.pass.cpp:22
+
+ assert(iter1 == iter2);
+ assert(iter1-- == iter2--);
----------------
Rather than comparing the two iterators, consider comparing the return values to `rv.begin() + some-index`, i.e. something like `assert(iter1-- == rv.begin() + 9);`.
================
Comment at: libcxx/test/std/ranges/range.factories/range.repeat.view/iterator/decrement.pass.cpp:30
+
+ static_assert(!std::is_reference_v<decltype(iter2++)>);
+ static_assert(std::is_reference_v<decltype(++iter2)>);
----------------
Can you check the exact type? That would be more specific and replace all the 3 `static_assert`s.
================
Comment at: libcxx/test/std/ranges/range.factories/range.repeat.view/iterator/increment.pass.cpp:33
+ static_assert(!std::is_reference_v<decltype(iter2++)>);
+ static_assert(std::is_reference_v<decltype(++iter2)>);
+
----------------
Nit: the static assertions below that check for the exact type make these redundant.
================
Comment at: libcxx/test/std/ranges/range.factories/range.repeat.view/iterator/member_typedefs.compile.pass.cpp:53
+
+ // If we're compiling for 32 bit or windows, int and long are the same size, so long long is the correct difference type.
+#if INTPTR_MAX == INT32_MAX || defined(_WIN32)
----------------
Would it be possible to use `std::uint64_t` or similar?
================
Comment at: libcxx/test/std/ranges/range.factories/range.repeat.view/iterator/minus.pass.cpp:23
+ std::ranges::repeat_view<int> v(0);
+ auto iter1 = std::next(v.begin(), 10);
+ auto iter2 = std::next(v.begin(), 10);
----------------
(applies throughout) If the iterator is random access, can you avoid using `next` and just do `v.begin() + 10`?
================
Comment at: libcxx/test/std/ranges/range.factories/range.repeat.view/iterator/minus.pass.cpp:26
+ assert(iter1 == iter2);
+ assert(iter1 - 5 != iter2);
+ assert(iter1 - 5 == std::ranges::prev(iter2, 5));
----------------
Nit: I don't think this check adds much value.
================
Comment at: libcxx/test/std/ranges/range.factories/range.repeat.view/iterator/minus.pass.cpp:27
+ assert(iter1 - 5 != iter2);
+ assert(iter1 - 5 == std::ranges::prev(iter2, 5));
+
----------------
Nit: consider using `next` from `begin` instead.
================
Comment at: libcxx/test/std/ranges/range.factories/range.repeat.view/iterator/minus.pass.cpp:29
+
+ static_assert(!std::is_reference_v<decltype(iter2 - 5)>);
+ }
----------------
Nit: if possible, it's better to check for the exact return type (applies throughout).
================
Comment at: libcxx/test/std/ranges/range.factories/range.repeat.view/iterator/minus_eq.pass.cpp:23
+ iter1 -= 5;
+ assert(iter1 != iter2);
+ assert(iter1 == std::ranges::prev(iter2, 5));
----------------
Same comments as the `minus.pass.cpp` file.
================
Comment at: libcxx/test/std/ranges/range.factories/range.repeat.view/iterator/plus.pass.cpp:26
+ assert(iter1 + 5 == iter2 + 5);
+ assert(5 + iter1 != iter2);
+ assert(5 + iter1 == 5 + iter2);
----------------
Same comments as for `operator-` -- I think it's better to compare to `v.begin() + N`.
================
Comment at: libcxx/test/std/ranges/range.factories/range.repeat.view/iterator/star.pass.cpp:18
+constexpr bool test() {
+ std::ranges::repeat_view<int> v(31);
+ auto iter = v.begin();
----------------
Can you add test cases to check a view that has bounds? I'd check a one-element view and a view of several elements.
================
Comment at: libcxx/test/std/ranges/range.factories/range.repeat.view/iterator/star.pass.cpp:23
+ assert(*iter == 31);
+ assert(std::addressof(*iter) == std::addressof(val));
+ }
----------------
Nit: I don't think it's necessary to use `addressof` in a test, it's used to make sure we don't have to deal with an overloaded `operator&` which cannot be the case here.
================
Comment at: libcxx/test/std/ranges/range.factories/range.repeat.view/iterator/subscript.pass.cpp:21
+ for (int i = 0; i < 100; ++i)
+ assert(iter[i] == 31);
+
----------------
Nit: use `std::ranges::all_of`?
================
Comment at: libcxx/test/std/ranges/range.factories/range.repeat.view/size.pass.cpp:11
+
+// constexpr auto size() const requires see below;
+
----------------
Nit: `s/see below/(!same_as<Bound, unreachable_sentinel_t>);/`.
================
Comment at: libcxx/test/std/ranges/range.factories/range.repeat.view/size.pass.cpp:17
+#include <cassert>
+#include <utility>
+
----------------
Nit: can these be sorted? (And I think also `s/<utility>/<iterator>/`)
================
Comment at: libcxx/test/std/ranges/range.factories/range.repeat.view/views_repeat.pass.cpp:11
+
+// views::repeat
+
----------------
Nit: it would be helpful to have the constraints here as well.
================
Comment at: libcxx/test/std/ranges/range.factories/range.repeat.view/views_repeat.pass.cpp:40
+
+constexpr bool test() {
+ assert(*std::views::repeat(33).begin() == 33);
----------------
Please add tests for piping the view with other views, see various `adaptor.pass.cpp` tests for examples.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D141699/new/
https://reviews.llvm.org/D141699
More information about the libcxx-commits
mailing list