[libcxx-commits] [PATCH] D141699: [libc++][ranges] Implement P2474R2(`views::repeat`).

[Yurong via Phabricator via libcxx-commits]

yronglin added a comment.

Thanks for your review! @var-const @philnik



================
Comment at: libcxx/test/std/ranges/range.adaptors/range.take/adaptor.pass.cpp:183
+#if TEST_STD_VER >= 23
+  // `views::take(repeat_view, n)` return a `repeat_view` when `repeat_view` models `sized_range`.
+  {
----------------
var-const wrote:
> Ultranit: `s/return/returns/` (here and below, and same in the `range.drop` test)
Thanks! done.


================
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);
----------------
var-const wrote:
> For each test case, can you add a very short comment to explain which case it's testing?
> ```
>   { // `(value)` constructor, non-const view
>     std::ranges::repeat_view<int> rv(0);
>     std::same_as<std::ranges::iterator_t<decltype(rv)>> decltype(auto) iter = rv.begin();
>     assert(*iter == 0);
>   }
>   { // `(value)` constructor, const view
>     const std::ranges::repeat_view<int> rv(0);
>     std::same_as<std::ranges::iterator_t<decltype(rv)>> decltype(auto) iter = rv.begin();
>     assert(*iter == 0);
>   }
>   // ...
> ```
done


================
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());
----------------
var-const wrote:
> yronglin wrote:
> > var-const wrote:
> > > yronglin wrote:
> > > > var-const wrote:
> > > > > 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?
> > > > Good catch, I'll try to add a new test case.
> > > Quick question, which file contains the newly-added test case?
> > In `star.pass.cpp`, I'd be glad to add a range-based-for in this file?
> > ```
> > for (const auto &v : repeat_view) {
> >    assert(v == some_value);
> > }
> > ```
> Yes, can you please add the test case to `star.pass.cpp`?
Thanks, added!


================
Comment at: libcxx/test/std/ranges/range.factories/range.repeat.view/iterator/plus_eq.pass.cpp:25
+  assert(iter1 != iter2);
+  assert(iter1 == std::ranges::next(iter2, 5));
+
----------------
var-const wrote:
> This `next` can be replaced by arithmetics.
Sorry, somehow, I have missed this before.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141699