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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added inline comments.


================
Comment at: libcxx/include/CMakeLists.txt:138
   __iterator/concepts.h
+  __iterator/counted_iterator.h
   __iterator/data.h
----------------
Did you run `libcxx/utils/generate_private_header_tests.py`?


================
Comment at: libcxx/include/__iterator/counted_iterator.h:39
+template<class>
+struct __counted_iterator_concept {};
+
----------------
Please add `_LIBCPP_TEMPLATE_VIS` here and other structs.


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


================
Comment at: libcxx/include/__iterator/counted_iterator.h:78
+
+  constexpr counted_iterator() requires default_initializable<_Iter> = default;
+
----------------
Please add `_LIBCPP_HIDE_FROM_ABI` here and other functions.


================
Comment at: libcxx/include/__iterator/counted_iterator.h:204
+  {
+    return __current_[__n];
+  }
----------------
How do you feel about adding `_LIBCPP_ASSERTS` to validate the precondtions?


================
Comment at: libcxx/include/__iterator/counted_iterator.h:220
+
+//   template<common_with<_Iter> _I2>
+//   friend constexpr strong_ordering operator<=>(
----------------
Can you add a TODO/FIXME to remind to implement this function?


================
Comment at: libcxx/test/std/iterators/predef.iterators/counted.iterator/iter_swap.pass.cpp:16
+//     iter_swap(const counted_iterator& x, const counted_iterator<I2>& y)
+//       noexcept(noexcept(ranges::iter_swap(x.current, y.current)));
+
----------------
I miss tests to validate the proper `noexcept` status.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106205