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

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const added inline comments.


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


================
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) -
----------------
yronglin wrote:
> yronglin wrote:
> > var-const wrote:
> > > 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?
> > I used `ranges::begin` previously, because `__range.__value_` was a private member of `repeat_view`. Nowadays, I make the `std::views::__take::__fn` and `std::views::__drop::__fn` as the friend of `repeat_view`, so we can access the private member `__range.__value_`. WDYT?
> done, use `views::repeat`.
I think staying close to the standard is preferable here (having to use friends is a little unfortunate, but seems like the right tradeoff).


================
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();
----------------
var-const wrote:
> 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`.
Seems not done -- I'd prefer the two test cases to only differ in whether the `rv` variable is const or not, but otherwise use same values (`0` and `10` in this case).


================
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();
----------------
var-const wrote:
> Same comment re. `s/33/20/` as above.
Seems not 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());
----------------
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?


================
Comment at: libcxx/test/std/ranges/range.factories/range.repeat.view/iterator/member_typedefs.compile.pass.cpp:17
+// using value_type = W;
+// using difference_type = see bellow:
+// If is-signed-integer-like<index-type> is true, the member typedef-name difference_type denotes
----------------
Nit: `s/bellow/below/`.


================
Comment at: libcxx/test/std/ranges/range.factories/range.repeat.view/iterator/member_typedefs.compile.pass.cpp:46
+      static_assert(std::is_signed_v<Iter::difference_type>);
+      static_assert(sizeof(Iter::difference_type) >= sizeof(std::int8_t));
+    }
----------------
Can this check be `==`? If I'm reading the wording correctly, if `index-type` is signed, then `difference_type` is the same type as `index-type`, so `std::int8_t` in this case?


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


================
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:
> (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.


================
Comment at: libcxx/test/std/ranges/range.factories/range.repeat.view/iterator/minus_eq.pass.cpp:20
+  std::ranges::repeat_view<int> v(10);
+  auto iter = std::next(v.begin(), 10);
+  iter -= 5;
----------------
You can probably remove this `next` (similarly in other `iterator` test files).


================
Comment at: libcxx/test/std/ranges/range.factories/range.repeat.view/iterator/minus_eq.pass.cpp:29
+
+int main(int, char**) { return 0; }
----------------
Missing calls to `test()`.


================
Comment at: libcxx/test/std/ranges/range.factories/range.repeat.view/iterator/star.pass.cpp:36
+    std::ranges::repeat_view<int, int> v(31, 1);
+    auto iter = v.begin();
+
----------------
Can you also check the value of `iter`?


================
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:
> 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`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141699