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

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb added a subscriber: mclow.lists.
cjdb added inline comments.


================
Comment at: libcxx/include/iterator:550
+template <class _InputIter, class _Distance,
+          class = typename enable_if<is_integral<decltype(_VSTD::__convert_to_integral(declval<_Distance>()))>::value>::type>
 inline _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX14
----------------
cjdb wrote:
> ldionne wrote:
> > What's the rationale for this change? I remember playing around the same area here: https://reviews.llvm.org/D81425, it might be useful to read that again for context. I don't have enough time to form an opinion again right now unfortunately.
> > 
> > If we decide to keep it, we should add a test. IIUC that would be considered a libc++ extension at this point based on glancing at https://reviews.llvm.org/D81425 again quickly.
> Making an unqualified call to `advance(i, s)` will yield `no matching function for call to '__convert_to_integral'` otherwise. That's definitely an implementation detail leaking out of the interface, and suppresses what I think is the more natural diagnostic: `no matching function for call to 'advance'`.
> 
> > If we decide to keep it, we should add a test. IIUC that would be considered a libc++ extension at this point based on glancing at https://reviews.llvm.org/D81425 again quickly.
> 
> I'll have a read and report my opinions here. I'm actually surprised that `std::advance` doesn't constrain this function on `Distance` being convertible to the iterator's distance type! That's probably an LWG defect there.
After looking at D81425, I think the goals are different. D81425 takes a not-to-spec `std::advance` that has `iterator_traits<I>::difference_type` as its parameter and makes it to-spec by changing it to `_Distance`. As much as I don't like that, it's to-spec and so is a Good Thing.

What I'm doing here is constraining when `std::advance` can participate in overload resolution, which prevents this [[ https://godbolt.org/z/sTbrd6bjW | unfortunate diagnostic spew ]]. There's always a diagnostic, but I'm changing what's presented to a user.

Yes, it's an extension, but I believe it's a conforming one, since the wording implies that `n` is convertible to an integral. See [[ http://eel.is/c++draft/alg.foreach#16 | std::for_each_n ]], for an example (all the `_n` algorithms already do this), which @mclow.lists points out. I've also now sent off an LWG defect asking for a mandates paragraph to be applied retroactively all the way back to C++98, so hopefully this won't be an extension come year-end.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101922