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

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const accepted this revision.
var-const added a comment.

LGTM with the remaining comments addressed (and a green CI). Feel free to ping me directly if you have any questions re. my comments. Thanks!



================
Comment at: libcxx/include/__fwd/repeat_view.h:45
+namespace __repeat {
+struct __fn {
+  template <class _Tp>
----------------
yronglin wrote:
> var-const wrote:
> > Does `__fn` have to be here and not in the main `repeat_view.h` file? It's not a forward declaration, it's a definition.
> Yeah, because in `drop_view.h` and `take_view.h` called `views::repeat(...)`, should we merge this file and the main `repeat_view.h` ?
Honestly, I don't have a strong preference here. @philnik What do you think? I don't think we had a precedent for defining the function objects in a `fwd` file (and IMO it makes it not really a forward file), so it feels perhaps a little "inelegant" (to me). Do you think this results in a meaningful reduction of compilation times?


================
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`.
+  {
----------------
Ultranit: `s/return/returns/` (here and below, and same in the `range.drop` test)


================
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);
----------------
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);
  }
  // ...
```


================
Comment at: libcxx/test/std/ranges/range.factories/range.repeat.view/begin.pass.cpp:29
+  {
+    std::ranges::repeat_view<int> rv(42);
+    std::same_as<std::ranges::iterator_t<decltype(rv)>> decltype(auto) iter = rv.begin();
----------------
Hmm, does this case differ from `std::ranges::repeat_view<int> rv(0);` other than in which value is being repeated? (same for the const version below)


================
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());
----------------
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`?


================
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));
+
----------------
This `next` can be replaced by arithmetics.


================
Comment at: libcxx/test/std/ranges/range.factories/range.repeat.view/views_repeat.pass.cpp:101
+
+  // bound && as_rvalue_view
+  {
----------------
There is no need to check all the possible view combinations, it would be hard to maintain and I don't think it meaningfully increases test coverage.


================
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);
----------------
yronglin wrote:
> var-const wrote:
> > var-const wrote:
> > > Please add tests for piping the view with other views, see various `adaptor.pass.cpp` tests for examples.
> > For consistency, please make sure we have the same coverage as various other `adaptor.pass.cpp` test files. See e.g. `test/std/ranges/range.adaptors/range.split/adaptor.pass.cpp` and make sure all the relevant test cases from that file are also applied to `repeat_view`.
> Okay, I have add test for `repeat_view` piping with adaptors,  except `join_view`.
I think testing with just a couple of different adaptors (e.g. `drop` and `transform`) is completely sufficient -- I'd remove the rest because it will be hard to maintain going forward.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141699