Skip to content
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

Add a note about overflowing in the RangeFrom iterator #32592

Merged
merged 1 commit into from
Apr 14, 2016

Conversation

tbu-
Copy link
Contributor

@tbu- tbu- commented Mar 29, 2016

No description provided.

@tbu-
Copy link
Contributor Author

tbu- commented Mar 29, 2016

@tbu-
Copy link
Contributor Author

tbu- commented Apr 13, 2016

r? @alexcrichton

@GuillaumeGomez
Copy link
Member

I'm not sure adding a note like this is the best solution (which would be to fix the issue :p).

@mitaa
Copy link
Contributor

mitaa commented Apr 13, 2016

see also #25696, #25708

@@ -1539,6 +1539,10 @@ impl<Idx: PartialOrd<Idx>> Range<Idx> {
///
/// See the [`contains()`](#method.contains) method for its characterization.
///
/// Note: No overflow checking is done for the iterator implementation; if you
/// use an integer range and the integer overflows, it will panic in debug mode
/// and create an endless loop in release mode.
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps the wording can be tweaked here slightly along the lines of:

  • It will likely panic in debug mode, but isn't necessarily guaranteed to do so at this time.
  • This will only create an endless loop for the range 0.., right? Something like 1.. will overflow to 0 at some point which should stop iteration?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it's always an endless loop (if it doesn't panic), the implementation does not have a path that returns None: https://github.com/tbu-/rust/blob/ad9f9a563ed82df5ea82bc32497dd986614bdee7/src/libcore/iter.rs#L4642-L4654.

Copy link
Member

Choose a reason for hiding this comment

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

Ah right. I guess I'm basically thinking we want to word this in such a way so that we're not committing to anything, just noting the current behavior.

@tbu- tbu- force-pushed the pr_range_from_overflow branch from ad9f9a5 to 6acd90f Compare April 13, 2016 18:09
@tbu-
Copy link
Contributor Author

tbu- commented Apr 13, 2016

@alexcrichton Reworded the note a bit. Is that fine?

@alexcrichton
Copy link
Member

@bors: r+ 6acd90f

Thanks!

@bors
Copy link
Contributor

bors commented Apr 13, 2016

⌛ Testing commit 6acd90f with merge 2b60207...

bors added a commit that referenced this pull request Apr 13, 2016
Add a note about overflowing in the `RangeFrom` iterator
@bors bors merged commit 6acd90f into rust-lang:master Apr 14, 2016
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.

5 participants