-
Notifications
You must be signed in to change notification settings - Fork 13.7k
Closed
Labels
E-easyCall for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue.Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue.
Description
The standard wishlist
fn main() {
let mut it = 255u8..;
println!("{:?}", it.next());
}
no longer panics, but AFAICT from the code this is not due to any deliberate change. The Iterator implementation is start.add_one()/swap, and add_one() is self + 1. This sounds like some overflow checks are being lost somehow.
Metadata
Metadata
Assignees
Labels
E-easyCall for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue.Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue.
Type
Projects
Milestone
Relationships
Development
Select code repository
Activity
TimNN commentedon Aug 29, 2016
related: #35589
durka commentedon Aug 31, 2016
This was indeed caused by MIR:
cc @eddyb
eddyb commentedon Aug 31, 2016
Missing
#[rustc_inherit_overflow_checks]
on theStep
methods that do arithmetic (add_one
andsub_one
), introduced in #34530.ashrwin commentedon Sep 1, 2016
Can I work on this ?
eddyb commentedon Sep 1, 2016
@ashrko619 Sure! The only change needed here is adding
#[rustc_inherit_overflow_checks]
before those 2 methods in all 3 implementations ofStep
and writing a test (not sure if it can be#[test]
- cc @rust-lang/libs).ashrwin commentedon Sep 1, 2016
@eddyb Thanks. About the test - why shouldn't it be
#[test]
?eddyb commentedon Sep 1, 2016
I don't know where we can put a
#[test]
for this, that's all. Also, it's dependent on having it in a separate crate, so#[test]
might invalidate that requirement (i.e. it would have a local copy ofStep
).However, it looks like
libcoretest
has the tests and it's a separate crate, so you should be able to add a test in that file.Step
impls #36365matthew-piziak commentedon Sep 9, 2016
@ashrko619 Any update on this issue? If not I might like to work on it.
ashrwin commentedon Sep 11, 2016
@matthew-piziak Feel free to work on it. I have been caught up at work this week :)
Auto merge of #36365 - matthew-piziak:silent-overflow, r=eddyb
cramertj commentedon Jan 24, 2017
@matthew-piziak Is issue fixed? I noticed your PR said "part of"-- is there more that needs to be done?
matthew-piziak commentedon Jan 24, 2017
@cramertj I think we're done here. The "part of" idiom is just to identify the parent of the PR.
@talchas @eddyb What still needs to be done for this issue, if anything?
brson commentedon Feb 24, 2017
Closing per @matthew-piziak !