Skip to content

Fix example code for unreachable! #19360

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
Nov 28, 2014
Merged

Fix example code for unreachable! #19360

merged 1 commit into from
Nov 28, 2014

Conversation

olivren
Copy link
Contributor

@olivren olivren commented Nov 27, 2014

The previous code was giving an incorrect result (not x/3).

Also, this function does not work with signed integers. It now accepts u32 instead of i32.

The previous code was giving an incorrect result (not x/3).
@rust-highfive
Copy link
Contributor

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

/// fn divide_by_three(x: u32) -> u32 { // one of the poorest implementations of x/3
/// for i in std::iter::count(0_u32, 1) {
/// if 3*i < i { panic!("u32 overflow"); }
/// if x < 3*i { return i-1; }
Copy link
Contributor

Choose a reason for hiding this comment

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

<= 3*i should be sufficient (and more clear), no? Using checked_mul would also make more sense to me than the explicit panic guard.

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, with <= 3*i, this gives:

0/3=0
1/3=1
2/3=1
3/3=1
4/3=2
5/3=2
6/3=2

Copy link
Contributor

Choose a reason for hiding this comment

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

Derp, yes.

bors added a commit that referenced this pull request Nov 28, 2014
The previous code was giving an incorrect result (not x/3).

Also, this function does not work with signed integers. It now accepts `u32` instead of `i32`.
@bors bors closed this Nov 28, 2014
@bors bors merged commit f01cbaa into rust-lang:master Nov 28, 2014
lnicola added a commit to lnicola/rust that referenced this pull request Apr 28, 2025
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