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

Louis Dionne via Phabricator via libcxx-commits libcxx-commits at lists.llvm.org
Sat May 8 09:36:15 PDT 2021

ldionne added inline comments.

Comment at: libcxx/include/__iterator/primitives.h:10
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>
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_; }

  rG LLVM Github Monorepo



More information about the libcxx-commits mailing list