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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/include/__iterator/primitives.h:10
+
+#ifndef _LIBCPP___ITERATOR_PRIMITIVES_H
+#define _LIBCPP___ITERATOR_PRIMITIVES_H
----------------
Why is this header named that way? Shouldn't this be `__iterator/advance.h` to be consistent with what we've done so far? We've never named headers after the section of the standard that they roughly represent, and I don't think we should start doing that. Section names can change, they don't necessarily best represent what's in the file, and it goes against what we've started doing consistently.


================
Comment at: libcxx/include/__iterator/primitives.h:39
+  template <signed_integral _Tp>
+  [[nodiscard]] static constexpr make_unsigned_t<_Tp> __abs(_Tp const __n) noexcept {
+    auto const __unsigned_n = __to_unsigned_like(__n);
----------------
Drop `[[nodiscard]]` (as discussed offline). Also applies to most other places where you use it.

Also, should this be promoted to a more general utility?

And also, this may be obvious to you, but  why don't we simply use `std::abs`? Is it because http://eel.is/c++draft/iterator.primitives#range.iter.op.advance-6.1.1 uses absolute value in math notation (which is well-defined for all inputs), whereas `std::abs` is UB if the absolute value can't be represented in the return type?


================
Comment at: libcxx/include/__iterator/primitives.h:60-70
+      while (__n > 0) {
+        --__n;
+        ++__i;
+      }
+
+      // Otherwise, decrements `i` by `-n`.
+      if constexpr (bidirectional_iterator<_Ip>) {
----------------
I understand why this works, but I find it's a slightly confusing way to write this. Instead, Maybe `else if constexpr (is_bidirectional_iterator<_Ip>) { handle bidirectional case } else { handle-forward-only-case }`.

It's not as clever, but maybe easier to understand?


================
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
----------------
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.


================
Comment at: libcxx/test/support/test_iterators.h:727
+
+  [[nodiscard]] constexpr I base() const& requires std::copyable<I> { return base_; }
+
----------------
`s/[[nodiscard]]//g`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101922