[libcxx-commits] [PATCH] D101922: [libcxx][iterator] adds `std::ranges::advance`

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

Quuxplusone added inline comments.


================
Comment at: libcxx/include/__iterator/primitives.h:99
+  template <input_or_output_iterator _Ip, sentinel_for<_Ip> _Sp>
+  [[nodiscard]] constexpr iter_difference_t<_Ip> operator()(_Ip& __i, iter_difference_t<_Ip> __n, _Sp __bound) const {
+    _LIBCPP_ASSERT(__n >= 0 || (bidirectional_iterator<_Ip> && same_as<_Ip, _Sp>),
----------------
cjdb wrote:
> zoecarver wrote:
> > cjdb wrote:
> > > zoecarver wrote:
> > > > This is a value we likely actually do want to discard.
> > > Please elaborate?
> > If we did the thing I wish we did in C++ and functions were nodiscard by default, there would be a `[[discardable_result]]` attribute instead. 
> > 
> > If that were the case, this function would be one of the ones that's marked with `[[discardable_result]]`, because there are valid reasons to use this function and discard the result. That's probably the most common use, in fact. 
> > 
> > In other words, this function is one of the ones where we don't actually need the nodiscard attribute. 
> > In other words, this function is one of the ones where we don't actually need the nodiscard attribute.
> 
> What I meant was "please elaborate on why you feel this function's result should be considered discardable".
`std::ranges::advance(i, s); // advance i to s`
This is the //normal and expected// use of the function: for its side effect. It's just like `std::advance`. When we get to `std::ranges::next`, you'll have a stronger case for `_LIBCPP_NODISCARD_EXT`.

Also, again, please make sure to use `_LIBCPP_NODISCARD_EXT` for things that are non-nodiscard according to the Standard Itself. We can mark them, but we need to be clear that it's a libc++ extension. libc++ uses raw `[[nodiscard]]` only in places that the Standard Itself uses `[[nodiscard]]` — a rare occurrence.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101922