[libcxx-commits] [PATCH] D117240: [libc++] Fix bug in ranges::advance and refactor the tests

[Louis Dionne via Phabricator via libcxx-commits]

ldionne created this revision.
ldionne requested review of this revision.
Herald added a project: libc++.
Herald added a subscriber: libcxx-commits.
Herald added a reviewer: libc++.

In `ranges::advance(iter, n, bound)`, we'd incorrectly handle the case
where bound < iter and n is 0:

  int a[10];
  int *p = a+5;
  int *bound = a+3;
  std::ranges::advance(p, 0, bound);
  assert(p - a == 5); // we'd return 3 before this patch

This was caused by an incorrect handling of 0 inside __magnitude_eq.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D117240

Files:
  libcxx/include/__iterator/advance.h
  libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.advance/iterator_count_sentinel.pass.cpp
  libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.prev/iterator_count_sentinel.pass.cpp


Index: libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.prev/iterator_count_sentinel.pass.cpp
===================================================================
--- libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.prev/iterator_count_sentinel.pass.cpp
+++ libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.prev/iterator_count_sentinel.pass.cpp
@@ -28,9 +28,7 @@
   assert(base(result) == expected);
 }
 
-// TODO: Re-enable once we fix the bug in ranges::advance
 constexpr bool test() {
-#if 0
   int range[] = {0, 1, 2, 3, 4, 5, 6, 7, 8, 9};
 
   for (int size = 0; size != 10; ++size) {
@@ -42,7 +40,6 @@
       check<int*>(                        range, range+size, n, expected);
     }
   }
-#endif
 
   return true;
 }
Index: libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.advance/iterator_count_sentinel.pass.cpp
===================================================================
--- libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.advance/iterator_count_sentinel.pass.cpp
+++ libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.advance/iterator_count_sentinel.pass.cpp
@@ -145,11 +145,10 @@
         // Note that we can only test ranges::advance with a negative n for iterators that
         // are sized sentinels for themselves, because ranges::advance is UB otherwise.
         // In particular, that excludes bidirectional_iterators since those are not sized sentinels.
-        // TODO: Enable these tests once we fix the bug in ranges::advance
-        // int* expected = n > size ? range : range + size - n;
-        // check_backward<random_access_iterator<int*>>(range, range+size, -n, expected);
-        // check_backward<contiguous_iterator<int*>>(   range, range+size, -n, expected);
-        // check_backward<int*>(                        range, range+size, -n, expected);
+        int* expected = n > size ? range : range + size - n;
+        check_backward<random_access_iterator<int*>>(range, range+size, -n, expected);
+        check_backward<contiguous_iterator<int*>>(   range, range+size, -n, expected);
+        check_backward<int*>(                        range, range+size, -n, expected);
       }
     }
   }
Index: libcxx/include/__iterator/advance.h
===================================================================
--- libcxx/include/__iterator/advance.h
+++ libcxx/include/__iterator/advance.h
@@ -74,10 +74,20 @@
 
 struct __fn final : private __function_like {
 private:
+  // Returns |a| >= |b|
   template <class _Tp>
   _LIBCPP_HIDE_FROM_ABI
-  static constexpr _Tp __magnitude_geq(_Tp __a, _Tp __b) noexcept {
-    return __a < 0 ? (__a <= __b) : (__a >= __b);
+  static constexpr bool __magnitude_geq(_Tp __a, _Tp __b) {
+    if (__a < 0)
+      if (__b < 0)
+        return __a <= __b;
+      else
+        return __a + __b <= 0;
+    else
+      if (__b < 0)
+        return __a + __b >= 0;
+      else
+        return __a >= __b;
   }
 
   template <class _Ip>


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D117240.399732.patch
Type: text/x-patch
Size: 3074 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/libcxx-commits/attachments/20220113/295faf7d/attachment-0001.bin>