Skip to content

Improve documentation of next_power_of_two #40706

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.

Sign up for GitHub

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 8 commits into from
Jun 8, 2017
Merged

Improve documentation of next_power_of_two #40706

Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
f7c641b
Improve documentation of next_power_of_two
irfanhudda Mar 21, 2017
04e9c20
next_power_of_two panic on overflow
irfanhudda Apr 19, 2017
f7bd370
remove redundant wrapping_sub
irfanhudda Apr 20, 2017
8f9caff
Add helper functions for `next_power_of_two` and `checked_next_power_…
irfanhudda Apr 28, 2017
67684a3
Add comment about overflow behavior for next_power_of_two
irfanhudda Apr 28, 2017
ed24829
Simplify `checked_next_power_of_two` function
irfanhudda Apr 28, 2017
93219a2
Add comments to explain helper functions
irfanhudda May 23, 2017
18fadb6
Simplify helper functions
irfanhudda May 31, 2017
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
14 changes: 11 additions & 3 deletions src/libcore/num/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2328,7 +2328,7 @@ macro_rules! uint_impl {
}

/// Returns the smallest power of two greater than or equal to `self`.
/// Unspecified behavior on overflow.
/// More details about overflow behavior can be found in [RFC 560].
Copy link

Choose a reason for hiding this comment

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

I'd leave the old sentence and put the new one after it.

Reading through a long RFC just to figure out what happens on overflow may be tedious. :)
It should be clear that on overflow something unspecified will happen, but if the reader wants to find out more - there's a link to the RFC.

Copy link
Contributor

@hanna-kruppe hanna-kruppe Mar 21, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Is it actually true that the behavior is unspecified? For normal arithmetic at least, the RFC used to state that behavior was unspecified, but soon afterwards it was specified as "panic in debug, wrapping in release" (slightly simplified).

Edit: The RFC doesn't actually mention next_power_of_two specifically. I suppose the behavior referred to is just the general overflow behavior, then?

Copy link
Contributor

Choose a reason for hiding this comment

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

By the way I agree that we should not just link to an RFC (which is not written for end users, and very much a historic document). But the old sentence is plainly wrong AFAIK.

Copy link

Choose a reason for hiding this comment

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

Looks like it doesn't panic even in debug mode: https://is.gd/pmGoPY
Although by "unspecified behavior" I take it could possibly panic, but doesn't have to.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, this doesn't match what was discussed in #18604, specifically #18604 (comment) indicates it should panic in debug mode.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that linking to RFC might not be a good idea.

From what I understand about overflow behavior (correct me if I'm wrong)

  1. Compiler can detect literal overflow at compile time, this can be disabled using
    #[allow(overflowing_literals)] https://is.gd/M6Oxnf
  2. In debug mode, compiler by default places some checks which panics on overflow. While in release mode it wraps on overflow. https://is.gd/gMmhFo
  3. Overflow checking can be turned off at compile time using -Z force-overflow-checks=off flag.

I could not find any page in doc which documents behavior on overflow so I linked to RFC. Let me know if there is a page which should be linked.

Or we could just remove this section because overflow behavior across the whole program should now be consistent

///
/// # Examples
///
Expand All @@ -2338,12 +2338,18 @@ macro_rules! uint_impl {
/// assert_eq!(2u8.next_power_of_two(), 2);
/// assert_eq!(3u8.next_power_of_two(), 4);
/// ```
///
/// [RFC 560]: https://github.com/rust-lang/rfcs/blob/master/text/0560-integer-overflow.md
#[stable(feature = "rust1", since = "1.0.0")]
#[inline]
pub fn next_power_of_two(self) -> Self {
let bits = size_of::<Self>() * 8;
let one: Self = 1;
one << ((bits - self.wrapping_sub(one).leading_zeros() as usize) % bits)
if self == 0 {
1
} else {
one << (bits - self.wrapping_sub(one).leading_zeros() as usize)
Copy link

Choose a reason for hiding this comment

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

Technically, you can replace self.wrapping_sub(one) with (self - one).
I think that'd make the intentions clearer (we just want to subtract one; we don't care about wrapping).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@stjepang Thanks for the suggestion.

}
}

/// Returns the smallest power of two greater than or equal to `n`. If
Expand All @@ -2361,7 +2367,9 @@ macro_rules! uint_impl {
/// ```
#[stable(feature = "rust1", since = "1.0.0")]
pub fn checked_next_power_of_two(self) -> Option<Self> {
let npot = self.next_power_of_two();
let bits = size_of::<Self>() * 8;
let one: Self = 1;
let npot = one << ((bits - self.wrapping_sub(one).leading_zeros() as usize) % bits);
Copy link

Choose a reason for hiding this comment

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

Isn't this going to panic if self == 0?
We need a check for this condition first, the same way you wrote one in fn next_power_of_two.

Also, I'd personally find the following slightly easier to understand (but I don't mind the current code either):

let shift = bits - self.wrapping_sub(one).leading_zeros() as usize;
if shift < bits {
    Some(one << shift)
} else {
    None
}

Copy link

Choose a reason for hiding this comment

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

By the way, do we have tests for next_power_of_two and checked_next_power_of_two?
If not, it'd be a good idea to write some that check for corner cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are tests for next_power_of_two and checked_next_power_of_two https://github.com/rust-lang/rust/blob/master/src/libstd/num.rs#L153

if npot >= self {
Some(npot)
} else {
Expand Down