[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 !



================
Comment at: libcxx/include/__fwd/repeat_view.h:45
+namespace __repeat {
+struct __fn {
+  template <class _Tp>
----------------
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` ?


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


================
Comment at: libcxx/test/std/ranges/range.factories/range.repeat.view/iterator/minus.pass.cpp:55
+      assert(iter1 - iter2 == 5);
+#if INTPTR_MAX == INT32_MAX || defined(_WIN32)
+      static_assert(std::same_as<decltype(iter1 - iter2), long long>);
----------------
var-const wrote:
> This is brittle. Can we check against something like `ptrdiff_t` instead of "concrete" types? If not, I'd rather remove the check than do platform-specific definitions.
I agree `ptrdiff_t` is better


================
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);
----------------
var-const wrote:
> var-const wrote:
> > (applies throughout) If the iterator is random access, can you avoid using `next` and just do `v.begin() + 10`?
> Still not done -- I think you can completely remove `next` from this file and just use arithmetic operations on iterators.
sorry, I missed it earlier


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141699