[libcxx-commits] [libcxx] d27cbfa - [libc++] Fix bug in ranges::advance
Louis Dionne via libcxx-commits
libcxx-commits at lists.llvm.org
Thu Jan 27 07:58:04 PST 2022
Author: Louis Dionne
Date: 2022-01-27T10:57:54-05:00
New Revision: d27cbfa9d366c2f6620770f514f612aa1bb0ecb6
URL: https://github.com/llvm/llvm-project/commit/d27cbfa9d366c2f6620770f514f612aa1bb0ecb6
DIFF: https://github.com/llvm/llvm-project/commit/d27cbfa9d366c2f6620770f514f612aa1bb0ecb6.diff
LOG: [libc++] Fix bug in ranges::advance
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_geq.
Differential Revision: https://reviews.llvm.org/D117240
Added:
Modified:
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
Removed:
################################################################################
diff --git a/libcxx/include/__iterator/advance.h b/libcxx/include/__iterator/advance.h
index 831f88f462744..03418979ddbd0 100644
--- a/libcxx/include/__iterator/advance.h
+++ b/libcxx/include/__iterator/advance.h
@@ -73,12 +73,6 @@ namespace __advance {
struct __fn {
private:
- template <class _Tp>
- _LIBCPP_HIDE_FROM_ABI
- static constexpr _Tp __magnitude_geq(_Tp __a, _Tp __b) noexcept {
- return __a < 0 ? (__a <= __b) : (__a >= __b);
- }
-
template <class _Ip>
_LIBCPP_HIDE_FROM_ABI
static constexpr void __advance_forward(_Ip& __i, iter_
diff erence_t<_Ip> __n) {
@@ -155,6 +149,12 @@ struct __fn {
// If `S` and `I` model `sized_sentinel_for<S, I>`:
if constexpr (sized_sentinel_for<_Sp, _Ip>) {
// If |n| >= |bound - i|, equivalent to `ranges::advance(i, bound)`.
+ // __magnitude_geq(a, b) returns |a| >= |b|, assuming they have the same sign.
+ auto __magnitude_geq = [](auto __a, auto __b) {
+ return __a == 0 ? __b == 0 :
+ __a > 0 ? __a >= __b :
+ __a <= __b;
+ };
if (const auto __M = __bound - __i; __magnitude_geq(__n, __M)) {
(*this)(__i, __bound);
return __n - __M;
diff --git a/libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.advance/iterator_count_sentinel.pass.cpp b/libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.advance/iterator_count_sentinel.pass.cpp
index 104e912e3c357..75f12802af7f5 100644
--- a/libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.advance/iterator_count_sentinel.pass.cpp
+++ b/libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.advance/iterator_count_sentinel.pass.cpp
@@ -119,6 +119,54 @@ static_assert(std::sized_sentinel_for<iota_iterator, iota_iterator>);
constexpr bool test() {
int range[] = {0, 1, 2, 3, 4, 5, 6, 7, 8, 9};
+ // Basic functionality test: advance forward, bound has the same type
+ {
+ int *p;
+ p = range+5; assert(std::ranges::advance(p, 0, range+7) == 0); assert(p == range+5);
+ p = range+5; assert(std::ranges::advance(p, 1, range+7) == 0); assert(p == range+6);
+ p = range+5; assert(std::ranges::advance(p, 2, range+7) == 0); assert(p == range+7);
+ p = range+5; assert(std::ranges::advance(p, 3, range+7) == 1); assert(p == range+7);
+ }
+
+ // Basic functionality test: advance forward, bound is not the same type and not assignable
+ {
+ int *p;
+ using ConstPtr = const int*;
+ p = range+5; assert(std::ranges::advance(p, 0, ConstPtr(range+7)) == 0); assert(p == range+5);
+ p = range+5; assert(std::ranges::advance(p, 1, ConstPtr(range+7)) == 0); assert(p == range+6);
+ p = range+5; assert(std::ranges::advance(p, 2, ConstPtr(range+7)) == 0); assert(p == range+7);
+ p = range+5; assert(std::ranges::advance(p, 3, ConstPtr(range+7)) == 1); assert(p == range+7);
+ }
+
+ // Basic functionality test: advance forward, bound has
diff erent type but assignable
+ {
+ const int *pc;
+ pc = range+5; assert(std::ranges::advance(pc, 0, range+7) == 0); assert(pc == range+5);
+ pc = range+5; assert(std::ranges::advance(pc, 1, range+7) == 0); assert(pc == range+6);
+ pc = range+5; assert(std::ranges::advance(pc, 2, range+7) == 0); assert(pc == range+7);
+ pc = range+5; assert(std::ranges::advance(pc, 3, range+7) == 1); assert(pc == range+7);
+ }
+
+ // Basic functionality test: advance backward, bound has the same type
+ // Note that we don't test advancing backward with a bound of a
diff erent type because that's UB
+ {
+ int *p;
+ p = range+5; assert(std::ranges::advance(p, 0, range+3) == 0); assert(p == range+5);
+ p = range+5; assert(std::ranges::advance(p, -1, range+3) == 0); assert(p == range+4);
+ p = range+5; assert(std::ranges::advance(p, -2, range+3) == 0); assert(p == range+3);
+ p = range+5; assert(std::ranges::advance(p, -3, range+3) == -1); assert(p == range+3);
+ }
+
+ // Basic functionality test: advance backward with an array as a sentinel
+ {
+ int* p;
+ p = range+5; assert(std::ranges::advance(p, 0, range) == 0); assert(p == range+5);
+ p = range+5; assert(std::ranges::advance(p, -1, range) == 0); assert(p == range+4);
+ p = range+5; assert(std::ranges::advance(p, -5, range) == 0); assert(p == range);
+ p = range+5; assert(std::ranges::advance(p, -6, range) == -1); assert(p == range);
+ }
+
+ // Exhaustive checks for n and range sizes
for (int size = 0; size != 10; ++size) {
for (int n = 0; n != 20; ++n) {
@@ -145,16 +193,15 @@ constexpr bool test() {
// 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);
}
}
}
- // regression-test that INT_MIN doesn't cause any undefined behavior
+ // Regression-test that INT_MIN doesn't cause any undefined behavior
{
auto i = iota_iterator{+1};
assert(std::ranges::advance(i, INT_MIN, iota_iterator{-2}) == INT_MIN+3);
diff --git a/libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.prev/iterator_count_sentinel.pass.cpp b/libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.prev/iterator_count_sentinel.pass.cpp
index d998f0ddb90c0..253194605fcfb 100644
--- a/libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.prev/iterator_count_sentinel.pass.cpp
+++ b/libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.prev/iterator_count_sentinel.pass.cpp
@@ -28,9 +28,7 @@ constexpr void check(int* first, int* last, std::iter_
diff erence_t<It> n, int* e
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 @@ constexpr bool test() {
check<int*>( range, range+size, n, expected);
}
}
-#endif
return true;
}
More information about the libcxx-commits
mailing list