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

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver marked 31 inline comments as done.
zoecarver added inline comments.


================
Comment at: libcxx/include/__iterator/counted_iterator.h:73
+  _Iter __current_ = _Iter();
+  iter_difference_t<_Iter> __count_ = 0;
+
----------------
Mordante wrote:
> These two are exposition only private members. Is there a reason why you made them public?
Yes, the hidden friends need to use them. 


================
Comment at: libcxx/test/std/iterators/predef.iterators/counted.iterator/base.pass.cpp:25
+  {
+    std::counted_iterator iter(cpp20_input_iterator<int*>{buffer}, 8);
+    assert(iter.base().base() == buffer);
----------------
ldionne wrote:
> Suggestion: templatize that test and call it with the various iterator archetypes to remove a bit of duplication.
This test we could easily do that for, but I like having it be as easy as possible to read. Your brain has to do the absolute minimum amount of work to understand what's going on here. It's harder to get wrong when all the types are concrete. 

Also, if there's a failure, say for input_iterators, it will be way easier to catch this way, then looking through a substitution failure/back trace. 


================
Comment at: libcxx/test/std/iterators/predef.iterators/counted.iterator/minus.pass.cpp:149
+    }
+    // Note: not using random_access_iterator here.
+    {
----------------
ldionne wrote:
> Can you add the reason why?
This is just to call out that the iterator doesn't have to be a random access iterator because we never actually touch the iterator itself when doing the minus operation (we subtract the counts). 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106205