[libcxx-commits] [PATCH] D106735: [libc++] Fix signed overflow inside ranges::advance.

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb requested changes to this revision.
cjdb added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/__iterator/advance.h:79
   _LIBCPP_HIDE_FROM_ABI
-  static constexpr _Tp __abs(_Tp __n) noexcept {
-    return __n < 0 ? -__n : __n;
----------------
Please don't remove `noexcept`.


================
Comment at: libcxx/include/__iterator/advance.h:156
   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>),
+    _LIBCPP_ASSERT((bidirectional_iterator<_Ip> && same_as<_Ip, _Sp>) || __n >= iter_difference_t<_Ip>(0),
                    "If `n < 0`, then `bidirectional_iterator<I> && same_as<I, S>` must be true.");
----------------
These explicit conversions shouldn't be necessary: zero is zero for all //integer-like// types. I also don't think swapping these two lines around is going to benefit anything.


================
Comment at: libcxx/include/__iterator/advance.h:161
       // If |n| >= |bound - i|, equivalent to `ranges::advance(i, bound)`.
-      if (const auto __M = __bound - __i; __abs(__n) >= __abs(__M)) {
+      if (iter_difference_t<_Ip> __M = __bound - __i; __magnitude_geq(__n, __M)) {
         (*this)(__i, __bound);
----------------
Please don't change the type.


================
Comment at: libcxx/include/__iterator/advance.h:164
         return __n - __M;
+      } else {
+        // Otherwise, equivalent to `ranges::advance(i, n)`.
----------------
This else is redundant, please remove it.


================
Comment at: libcxx/include/__iterator/advance.h:172
       // most `n` times.
-      while (__i != __bound && __n > 0) {
+      while (bool(__i != __bound) && __n > iter_difference_t<_Ip>(0)) {
         ++__i;
----------------
These explicit conversion-to-`bool`s aren't necessary.


================
Comment at: libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.advance/iterator_count_sentinel.pass.cpp:104
 
+struct Iota {
+    using difference_type = int;
----------------
Please match the naming conventions that the file uses.


================
Comment at: libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.advance/iterator_count_sentinel.pass.cpp:162
+    auto i = Iota{+1};
+    assert(std::ranges::advance(i, INT_MIN, Iota{-2}) == INT_MIN+3);
+    assert(i == Iota{-2});
----------------
I'd prefer we use `std::numeric_limits<Iota::difference_type>::min()`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106735