Skip to content

core: Fix size_hint for signed integer Range<T> iterators #24865

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 29, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 35 additions & 4 deletions src/libcore/iter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2407,12 +2407,14 @@ pub trait Step: PartialOrd {
/// `start` should always be less than `end`, so the result should never
/// be negative.
///
/// `by` must be > 0.
///
/// Returns `None` if it is not possible to calculate steps_between
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Explanation: "Result should never be negative" above implies this.

/// without overflow.
fn steps_between(start: &Self, end: &Self, by: &Self) -> Option<usize>;
}

macro_rules! step_impl {
macro_rules! step_impl_unsigned {
($($t:ty)*) => ($(
impl Step for $t {
#[inline]
Expand All @@ -2423,7 +2425,33 @@ macro_rules! step_impl {
#[allow(trivial_numeric_casts)]
fn steps_between(start: &$t, end: &$t, by: &$t) -> Option<usize> {
if *start <= *end {
Some(((*end - *start) / *by) as usize)
// Note: We assume $t <= usize here
Some((*end - *start) as usize / (*by as usize))
} else {
Some(0)
}
}
}
)*)
}
macro_rules! step_impl_signed {
($($t:ty)*) => ($(
impl Step for $t {
#[inline]
fn step(&self, by: &$t) -> Option<$t> {
(*self).checked_add(*by)
}
#[inline]
#[allow(trivial_numeric_casts)]
fn steps_between(start: &$t, end: &$t, by: &$t) -> Option<usize> {
if *start <= *end {
// Note: We assume $t <= isize here
// Use .wrapping_sub and cast to usize to compute the
// difference that may not fit inside the range of isize.
Some(
((*end as isize).wrapping_sub(*start as isize) as usize
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this wrapping_sub needed here because start is verified to be less than end?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you ask, I should probably insert a comment there.

It's needed because the length of a Range<i8> or Range<i64> etc may be larger than the maximum of the signed type. We compute a wrapping sub and cast to unsigned and get the correct value. This was the essence of the bug.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh so this is handling the case where you're calculating the steps_between something like usize::max_value() - 10 and 0? In that case I can see where the subtraction might legitimately overflow (but it's ok)!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I do agree though that a comment would be nice)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the test cases. It shouldn't be able to be larger than usize::MAX. (We guarantee ExactSizeIterator here and would have to revoke that).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ugh, my patch is totally broken because I've been thinking only about the signed case. Need to account for the unsigned case too, I understand if it's puzzling to read. I mean the cast to isize will be correct without overflow checks, but not with.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may want to add a few test cases that use integer types larger than i8 and i16 as well because they'll always fit into an isize right now.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm adding a test case for isize now. (-2..isize::MAX).size_hint(); was correct in optimized code already, but overflows in debug mode. Sad times.

/ (*by as usize))
)
} else {
Some(0)
}
Expand All @@ -2447,9 +2475,12 @@ macro_rules! step_impl_no_between {
)*)
}

step_impl!(usize u8 u16 u32 isize i8 i16 i32);
step_impl_unsigned!(usize u8 u16 u32);
step_impl_signed!(isize i8 i16 i32);
#[cfg(target_pointer_width = "64")]
step_impl_unsigned!(u64);
#[cfg(target_pointer_width = "64")]
step_impl!(u64 i64);
step_impl_signed!(i64);
#[cfg(target_pointer_width = "32")]
step_impl_no_between!(u64 i64);

Expand Down
6 changes: 6 additions & 0 deletions src/libcoretest/iter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
use core::iter::*;
use core::iter::order::*;
use core::iter::MinMaxResult::*;
use core::isize;
use core::usize;
use core::cmp;

Expand Down Expand Up @@ -758,6 +759,11 @@ fn test_range() {
assert_eq!((usize::MAX - 1..usize::MAX).size_hint(), (1, Some(1)));
assert_eq!((-10..-1).size_hint(), (9, Some(9)));
assert_eq!((-1..-10).size_hint(), (0, Some(0)));

assert_eq!((-70..58i8).size_hint(), (128, Some(128)));
assert_eq!((-128..127i8).size_hint(), (255, Some(255)));
assert_eq!((-2..isize::MAX).size_hint(),
(isize::MAX as usize + 2, Some(isize::MAX as usize + 2)));
}

#[test]
Expand Down