[libcxx-commits] [PATCH] D106205: [libcxx][ranges] Add `counted_iterator`.

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added a comment.
This revision is now accepted and ready to land.

LGTM pending comments and passing CI!



================
Comment at: libcxx/include/__iterator/counted_iterator.h:39
+template<class>
+struct __counted_iterator_concept {};
+
----------------
zoecarver wrote:
> Mordante wrote:
> > Please add `_LIBCPP_TEMPLATE_VIS` here and other structs.
> AFAIU this macro doesn't actually do anything useful, so I'm not going to be applying it. 
It doesn't do anything useful *in this context* because we do not explicitly instantiate the type in the dylib.


================
Comment at: libcxx/test/std/iterators/predef.iterators/counted.iterator/arrow.pass.cpp:22
+template<class Iter>
+concept ArrowtEnabled = requires(Iter& iter) {
+  iter.operator->();
----------------
Typo?


================
Comment at: libcxx/test/std/iterators/predef.iterators/counted.iterator/base.pass.cpp:32
+constexpr bool test() {
+  int buffer[8] = {1,2 ,3 , 4, 5, 6, 7, 8};
+
----------------
Weird comma separation.


================
Comment at: libcxx/test/std/iterators/predef.iterators/counted.iterator/count.pass.cpp:60
+
+  {
+    const std::counted_iterator iter(cpp20_input_iterator<int*>{buffer}, 8);
----------------
`// const versions`?


================
Comment at: libcxx/test/std/iterators/predef.iterators/counted.iterator/ctor.conv.pass.cpp:22
+
+class ConvertibleIter
+{
----------------
Consider using `ConvertibleTo<IterType>` instead? That way, in the tests below, we can see more clearly what types you're trying to test for convertibility, instead of having to look at the conversion operator below.


================
Comment at: libcxx/test/std/iterators/predef.iterators/counted.iterator/ctor.default.pass.cpp:21
+int main(int, char**) {
+  static_assert( std::default_initializable<std::counted_iterator<cpp17_input_iterator<int*>>>);
+  static_assert(!std::default_initializable<std::counted_iterator<cpp20_input_iterator<int*>>>);
----------------
We need to run the default constructor and make sure it initializes the iterator as it should.

Using `= default` to implement something is till an implementation, and we need to test that. For example, your test would pass if we never defined it at all, but users would get a linker error when they tried using the default ctor.


================
Comment at: libcxx/test/std/iterators/predef.iterators/counted.iterator/minus.iter.pass.cpp:91
+  }
+  // Note: not using random_access_iterator here.
+  {
----------------
I still don't understand this comment. Based on what you just told me, perhaps you should write this instead:

```
// no need to use a random_access_iterator since counted_iterator doesn't actually subtract the underlying iterators
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106205