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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone accepted this revision.
Quuxplusone added a comment.

(I didn't look at the tests.)



================
Comment at: libcxx/include/__iterator/counted_iterator.h:234
+  {
+    _LIBCPP_ASSERT(-__n <= __count_, "Cannot advance iterator back before begin.");
+    __current_ -= __n;
----------------
This assertion is... confusing. My impression is that `__count_ >= 0` is a class invariant. So `-__n > __count_` would happen only when the user is trying to decrease by a large negative value, e.g. `it -= -42;` when `it.count() < 42`.
So the assertion is(?) correct; but the message should be more like `"Attempted to advance a counted_iterator beyond the end of its range"`. (Compare the grammar to other `_LIBCPP_ASSERT`s, e.g. in `<list>`.)


================
Comment at: libcxx/include/__iterator/counted_iterator.h:263-266
+// TODO: implement this once three_way_comparable is landed.
+//   template<common_with<_Iter> _I2>
+//   friend constexpr strong_ordering operator<=>(
+//     const counted_iterator& __lhs, const counted_iterator<_I2>& __rhs);
----------------
This doesn't involve the `three_way_comparable` concept (just `common_with`), so couldn't you just land it now?


================
Comment at: libcxx/include/__iterator/counted_iterator.h:273
+  {
+    _LIBCPP_ASSERT(__i.__count_ > 0, "Iterator must not be past end of range.");
+    return ranges::iter_move(__i.__current_);
----------------
I recommend `"Attempted to iter_move a non-dereferenceable counted_iterator"`


================
Comment at: libcxx/include/__iterator/counted_iterator.h:283
+    _LIBCPP_ASSERT(__x.__count_ > 0 && __y.__count_ > 0,
+                   "Iterators must not be past end of range.");
+    return ranges::iter_swap(__x.__current_, __y.__current_);
----------------
I recommend `"Attempted to iter_swap a non-dereferenceable counted_iterator"`


================
Comment at: libcxx/include/__iterator/counted_iterator.h:107
+  _LIBCPP_HIDE_FROM_ABI
+  constexpr iter_difference_t<_Iter> count() const noexcept { return __count_; }
+
----------------
cjdb wrote:
> ldionne wrote:
> > Are you testing the `noexcept`-ness of this method in your tests? If not, you should, especially since it's in the spec.
> Why did the noexcept specifier removed? It's not a difficult one to get right and is arguably useful.
`noexcept` specifiers are useful if-and-only-if someone has a use for them. For any function whose noexceptness is never used, marking it `noexcept` is unhelpful — and in fact may be harmful, because (even though I agree //this one// is "easy to get right") as soon as you get even //one// `noexcept` wrong, you end up with a rogue `std::terminate`. It is possible to introduce a bug by adding `noexcept`; it is not possible to introduce a bug by leaving it off. Combine this with the general mantra "The best code is no code," and you end up with my standard advice: Use `noexcept` on move constructors to avoid the vector pessimization, and (in libc++) use it where it's mandated by the paper standard, but otherwise play it safe and keep it simple.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106205