Skip to content

Fix range sugar #20500

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
Jan 5, 2015
Merged

Fix range sugar #20500

merged 1 commit into from
Jan 5, 2015

Conversation

globin
Copy link
Contributor

@globin globin commented Jan 4, 2015

Range sugar was using the trait incorrectly plus the trait implementation was wrong.

Both had the arguments swapped which actually caused the tests to run but threw the debug_assert
in debug builds, causing normal builds of anything using the sugar to fail.

@rust-highfive
Copy link
Contributor

r? @pcwalton

(rust_highfive has picked a reviewer for you, use r? to override)

@huonw
Copy link
Member

huonw commented Jan 4, 2015

cc @nick29581, what's the intention of the Option return value of this trait? Is it intended to signal errors in this manner?

@huonw
Copy link
Member

huonw commented Jan 4, 2015

Thanks @globin for fixing this!

@nrc
Copy link
Member

nrc commented Jan 4, 2015

No, this is not right - the Option is None if for the case where it is impractical or impossible to calculate the size hint, e.g., for open ranges, or ranges over data types where calculating the difference is expensive or impossible.

So, this patch should just switch the params (the history there is the method used to be called difference or something, where it made sense to have the larger thing first) and continue with the debug_assert.

@nrc nrc assigned nrc and unassigned pcwalton Jan 4, 2015
@Thiez
Copy link
Contributor

Thiez commented Jan 4, 2015

Perhaps a and b should be given more informative names, such as start and finish or begin and end, or first_index and last_index. Having them named a and b was probably partially responsible for this buggy code.

And surely the step_impl_no_between! macro could be more intelligent, right now, on 32 bits, Step::steps_between(1i64, 5i64) will return None even though the result fits in a uint perfectly. Speaking of which, why does steps_between return the steps as a uint in the first place, rather than Self (which would always avoid overflow)?

@globin
Copy link
Contributor Author

globin commented Jan 4, 2015

I've just updated with new param names and the debug_assert! reintroduced.

@globin
Copy link
Contributor Author

globin commented Jan 4, 2015

Is steps_between supposed to allow start == end?

@nrc
Copy link
Member

nrc commented Jan 4, 2015

Yeah, I think start == end is fine

bors added a commit that referenced this pull request Jan 4, 2015
Fix range sugar

Reviewed-by: nick29581
@bors bors merged commit 5cc1738 into rust-lang:master Jan 5, 2015
@globin globin deleted the fix/range-sugar branch January 5, 2015 11:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants