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

[Zoe Carver via Phabricator via libcxx-commits]

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


================
Comment at: libcxx/include/__iterator/counted_iterator.h:234
+  {
+    _LIBCPP_ASSERT(-__n <= __count_, "Cannot advance iterator back before begin.");
+    __current_ -= __n;
----------------
Quuxplusone wrote:
> 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>`.)
Yes, I'll update the message to make it a bit clearer. Good idea. 


================
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_);
----------------
Quuxplusone wrote:
> I recommend `"Attempted to iter_move a non-dereferenceable counted_iterator"`
I think what I have now is a bit clearer. If we answer the question, "why is the iterator non-dereferenceable" then we get the message I have here. And I think that's actually a simpler message; it gets the user to their solution more quickly. 


================
Comment at: libcxx/include/__iterator/counted_iterator.h:107
+  _LIBCPP_HIDE_FROM_ABI
+  constexpr iter_difference_t<_Iter> count() const noexcept { return __count_; }
+
----------------
Quuxplusone wrote:
> 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.
> Why did the noexcept specifier removed? It's not a difficult one to get right and is arguably useful.

The noexcept specifier wasn't removed, this comment just moved. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106205