[libcxx-commits] [PATCH] D106923: [libcxx][ranges] Add `views::counted` CPO.

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/__ranges/counted.h:39
+  struct __fn {
+    template<class _Tp>
+      requires contiguous_iterator<decay_t<_Tp>>
----------------
Throughout, could you use `_Iter` instead of `_Tp` and perhaps `__it` instead of `__t`? I think it conveys the meaning a bit better.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.counted/counted.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
We need to test that `views::counted` is a CPO, see http://eel.is/c++draft/customization.point.object.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.counted/counted.pass.cpp:3
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
----------------
Can you add a test that `views::counted(iter, diff)` is SFINAEd out when `diff` is not convertible to the `iter_difference_t` of `iter`, per http://eel.is/c++draft/range.adaptors#range.counted-2?


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.counted/counted.pass.cpp:29
+      contiguous_iterator<int*> iter(buffer);
+      std::span<const int> s = std::views::counted(iter, 8);
+      assert(s.size() == 8);
----------------
I'm not immediately seeing why this is `std::span<const int>` and not `std::span<int>`, can you explain?


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.counted/counted.pass.cpp:73
+    }
+  }
+
----------------
Can you also add a test with a `forward_iterator`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106923