Improve documentation of next_power_of_two #40706

irfanhudda · 2017-03-21T16:49:07Z

Clarify overflow behavior of next_power_of_two.

Related Issue: #18604

rust-highfive · 2017-03-21T16:49:20Z

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

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

ghost · 2017-03-21T19:05:49Z

src/libcore/num/mod.rs

@@ -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].


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.

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?

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.

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.

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

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)

Compiler can detect literal overflow at compile time, this can be disabled using
#[allow(overflowing_literals)] https://is.gd/M6Oxnf

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

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

steveklabnik · 2017-03-23T20:33:22Z

@irfanhudda thanks for tackling this!

In this case, if this function doesn't do anything special with regards to overflow, we should just remove this sentence. If it does, than that's what we should document.

From the discussion above, it looks to me like there's nothing special here, so this wording should probably just be removed.

hanna-kruppe · 2017-03-23T20:40:09Z

@steveklabnik The playground @stjepang posted earlier (https://is.gd/pmGoPY) indicates that it does do something special, namely never panic. However, based on #18604 (comment) as well as general principles, I believe this is a bug (but maybe this should be brough up over there and discussed again).

steveklabnik · 2017-03-23T21:06:10Z

Let's get libs to decide. Tagging @rust-lang/libs

irfanhudda · 2017-03-23T21:18:38Z

@steveklabnik @rkruppe I have no idea why that example (https://is.gd/pmGoPY) should panic when there is no overflow. (in next_power_of_two function)

one << ((bits - self.wrapping_sub(one).leading_zeros() as usize) % bits)

Can you take a look at https://is.gd/tkY0UX (this has warning for literal overflow)

Or for case with panic https://is.gd/EJq9gM

I believe that behavior here is consistent (Warn for literal overflow and panic when some operation results in overflow)

hanna-kruppe · 2017-03-23T21:25:54Z

Can you elaborate? There is overflow: The next power of two after 255 is 256, which overflows just as much as 255+1 is (only the implementation does something slightly more sensible, ensuring the result after wrapping is still some power of two). That overflowing literals produce a warning rather than a hard error is mostly a historical accident AFAIK, and not relevant at any rate because 255u8 is in range and 255u8.next_power_of_two() isn't a literal.

irfanhudda · 2017-03-23T21:33:28Z

@rkruppe Your are correct that next power of 255 is 256. But the implementation of next_power_of_two while executing has no overflow because the line

one << ((bits - self.wrapping_sub(one).leading_zeros() as usize) % bits)

takes modulo bits so the result returned is 1<<0.

Let me know if I'm missing something here

EDIT:
Looking at function src/libcore/num/mod.rs#L2343

        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)
        }

I don't think there can be overflow in any statement so there is no panic it just wraps around the next power of two

hanna-kruppe · 2017-03-23T21:46:26Z

Yes, that it doesn't panic is an entirely natural consequence of its implementation. I am arguing the implementation is flawed.

irfanhudda · 2017-03-23T21:50:31Z

@rkruppe Thanks for clearing that up.

alexcrichton · 2017-04-12T22:08:24Z

Discussed during libs triage the other day the conclusion was that next_power_of_two should panic in debug mode (e.g. 0xffu8.next_power_of_two() should panic) and it's a bug that it doesn't currently. @irfanhudda would you be interested in implementing the change?

irfanhudda · 2017-04-13T15:03:31Z

@alexcrichton I'm interested in implementing this change. Should I create a new pull request for it or continue working on this one?

alexcrichton · 2017-04-13T16:07:28Z

Nah it's ok to do it here, no worries!

Clarify overflow behavior of `next_power_of_two`. Related Issue: rust-lang#18604

ghost · 2017-04-19T18:46:39Z

src/libcore/num/mod.rs

-            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);


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 }

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.

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

ghost · 2017-04-19T18:48:48Z

src/libcore/num/mod.rs

+            if self == 0 {
+                1
+            } else {
+                one << (bits - self.wrapping_sub(one).leading_zeros() as usize)


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).

@stjepang Thanks for the suggestion.

scottmcm · 2017-04-20T03:32:34Z

This is correcting the debug mode behaviour of the method; is fixing the release mode behaviour of the method also in-scope for this change?

Like @rkruppe said earlier, the power of two after 255 is 256, so under "overflow panics in debug and wraps in release" it seems to me like 255u8.next_power_of_two() should be 0 in release, not the current 1.

Possible approach (based on https://graphics.stanford.edu/~seander/bithacks.html#RoundUpPowerOf2). Add the following private (especially with that name) helper:

    fn round_up_to_one_less_than_a_power_of_two(self) -> Self {
        let bits = std::mem::size_of::<Self>() as u32 * 8;
        let z = self.leading_zeros();
        (if z == bits { 0 as Self } else { !0 }).wrapping_shr(z)
    }

It's a well-behaved function with no overflow cases; it just sets all bits right of the first bit that's set.

Then next_power_of_two is

    fn next_power_of_two(self) -> Self {
        self
            .wrapping_sub(1)
            .round_up_to_one_less_than_a_power_of_two()
            .wrapping_add(if self == 0 { 1 } else { 0 })
            + 1
    }

with the obvious extension to checked_next_power_of_two (or a doesn't-exist-today-but-could wrapped_next_power_of_two).

As a bonus, this approach adds no new branches in the release assembly over the current implementation. (LLVM turns the conditionals into sext and select.)

alexcrichton · 2017-04-24T21:08:39Z

Seems reasonable to me to implement @rkruppe's suggestion, @irfanhudda would you be up for including that inline? It may also be worth expanding on the panic semantics a bit in addition to referencing the associated RFC.

hanna-kruppe · 2017-04-24T21:18:35Z

@rkruppe said earlier, the power of two after 255 is 256, so under "overflow panics in debug and wraps in release" it seems to me like 255u8.next_power_of_two() should be 0 in release, not the current 1.

I'm not actually sure if that's a good idea. The current behavior is a special case, yes, but it might be useful that the result is always a power of two even if there's overflow. Or maybe not. I don't know, I just want to clarify I'm not arguing for changing this behavior in release mode.

It may also be worth expanding on the panic semantics a bit in addition to referencing the associated RFC.

We agreed early on that linking to the RFC would not be good. I've actually found yet another outdated statement in the RFC text since then, and am now convinced the RFC needs to be either amended to reflect the status quo, or amended with a big fact disclaimer that it's totally inaccurate. (But even if that was done, I wouldn't want to link to it in user facing documentation.)

alexcrichton · 2017-04-24T21:20:38Z

Er sorry, mixed up the @-mention

scottmcm · 2017-05-04T20:26:19Z

oh so earlier it was also indicated that maybe we shouldn't link to the RFC here? What do you think about that?

Maybe it can say something more specific to this method? First stab: "Overflows for uN when self > (1 << N-1), panicking in debug mode and wrapping to 0 in release mode. (The latter is the only situation in which this method can return zero.)"

alexcrichton · 2017-05-11T22:29:46Z

We’re going to close this for now, to keep our queue clean, but if you get a chance to update this, please reopen!

irfanhudda · 2017-05-24T15:25:32Z

@alexcrichton I updated this branch but I'm not sure if I can open this pull request. Can you open this one?

steveklabnik · 2017-05-24T15:28:03Z

@irfanhudda i got it 👍

irfanhudda · 2017-05-24T18:24:27Z

Thanks @steveklabnik

I've added comments to helper functions to better explain their use. Let me know if any changes are required.

scottmcm · 2017-05-28T05:24:12Z

Hmm, @alexcrichton is right -- I proposed something overly complicated here.

@irfanhudda Here's a simplification, if you'd like: scottmcm@e87afa3 (based on your branch)

@scottmcm

Based on @scottmcm 's suggestion

irfanhudda · 2017-05-31T16:58:57Z

Thanks @scottmcm for suggestion.

Mark-Simulacrum · 2017-06-04T01:20:53Z

@alexcrichton I believe this is ready for another look.

alexcrichton · 2017-06-07T15:27:27Z

@bors: r+

Oops sorry, thanks @irfanhudda!

bors · 2017-06-07T15:27:28Z

📌 Commit 18fadb6 has been approved by alexcrichton

bors · 2017-06-07T18:12:18Z

⌛ Testing commit 18fadb6 with merge 65a2688...

Improve documentation of next_power_of_two Clarify overflow behavior of `next_power_of_two`. Related Issue: #18604

bors · 2017-06-07T21:22:05Z

💔 Test failed - status-travis

Mark-Simulacrum · 2017-06-07T21:26:23Z

@bors retry

\The job exceeded the maximum time limit for jobs, and has been terminated.

bors · 2017-06-07T22:42:56Z

⌛ Testing commit 18fadb6 with merge f8f86c1...

bors · 2017-06-07T22:48:12Z

💔 Test failed - status-travis

aidanhs · 2017-06-07T23:33:56Z

[00:00:00] Sending build context to Docker daemon 392.7 kB
[00:00:00] Step 1 : FROM ubuntu:16.04
[00:00:04] Pulling repository docker.io/library/ubuntu
[00:00:06] Error: Status 500 trying to pull repository library/ubuntu: "<html><body><h1>500 Server Error</h1>\nAn internal server error occured.\n</body></html>\n\n"

Docker hub failures? Working fine locally, let's try again.

@bors retry

bors · 2017-06-08T01:07:15Z

⌛ Testing commit 18fadb6 with merge d8d5592...

Improve documentation of next_power_of_two Clarify overflow behavior of `next_power_of_two`. Related Issue: #18604

bors · 2017-06-08T03:46:11Z

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing d8d5592 to master...

rust-highfive assigned brson Mar 21, 2017

ghost reviewed Mar 21, 2017

View reviewed changes

steveklabnik added I-needs-decision Issue: In need of a decision. I-nominated T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Mar 23, 2017

hicqu mentioned this pull request Apr 9, 2017

use buildin next_power_of_two hicqu/fifo-rs#1

Closed

Mark-Simulacrum added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Apr 14, 2017

Improve documentation of next_power_of_two

f7c641b

Clarify overflow behavior of `next_power_of_two`. Related Issue: rust-lang#18604

irfanhudda force-pushed the doc-next-power-of-two branch from 3570506 to f7c641b Compare April 18, 2017 16:09

next_power_of_two panic on overflow

04e9c20

ghost suggested changes Apr 19, 2017

View reviewed changes

remove redundant wrapping_sub

f7bd370

alexcrichton removed I-needs-decision Issue: In need of a decision. I-nominated labels Apr 24, 2017

alexcrichton removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 4, 2017

alexcrichton closed this May 11, 2017

Add comments to explain helper functions

93219a2

steveklabnik reopened this May 24, 2017

Mark-Simulacrum added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 28, 2017

Simplify helper functions

18fadb6

Based on @scottmcm 's suggestion

bors added a commit that referenced this pull request Jun 7, 2017

Auto merge of #40706 - irfanhudda:doc-next-power-of-two, r=alexcrichton

65a2688

Improve documentation of next_power_of_two Clarify overflow behavior of `next_power_of_two`. Related Issue: #18604

bors added a commit that referenced this pull request Jun 8, 2017

Auto merge of #40706 - irfanhudda:doc-next-power-of-two, r=alexcrichton

d8d5592

Improve documentation of next_power_of_two Clarify overflow behavior of `next_power_of_two`. Related Issue: #18604

bors merged commit 18fadb6 into rust-lang:master Jun 8, 2017

Mark-Simulacrum mentioned this pull request Jun 15, 2017

next_power_of_two should panic on overflow #18604

Closed

Improve documentation of next_power_of_two #40706

Improve documentation of next_power_of_two #40706

Conversation

irfanhudda commented Mar 21, 2017

rust-highfive commented Mar 21, 2017

Choose a reason for hiding this comment

hanna-kruppe Mar 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steveklabnik commented Mar 23, 2017

hanna-kruppe commented Mar 23, 2017

steveklabnik commented Mar 23, 2017 • edited Loading

irfanhudda commented Mar 23, 2017 • edited Loading

hanna-kruppe commented Mar 23, 2017

irfanhudda commented Mar 23, 2017 • edited Loading

hanna-kruppe commented Mar 23, 2017

irfanhudda commented Mar 23, 2017

alexcrichton commented Apr 12, 2017

irfanhudda commented Apr 13, 2017

alexcrichton commented Apr 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottmcm commented Apr 20, 2017

alexcrichton commented Apr 24, 2017

hanna-kruppe commented Apr 24, 2017

alexcrichton commented Apr 24, 2017

scottmcm commented May 4, 2017

alexcrichton commented May 11, 2017

irfanhudda commented May 24, 2017

steveklabnik commented May 24, 2017

irfanhudda commented May 24, 2017

scottmcm commented May 28, 2017

irfanhudda commented May 31, 2017

Mark-Simulacrum commented Jun 4, 2017

alexcrichton commented Jun 7, 2017

bors commented Jun 7, 2017

bors commented Jun 7, 2017

bors commented Jun 7, 2017

Mark-Simulacrum commented Jun 7, 2017 • edited Loading

bors commented Jun 7, 2017

bors commented Jun 7, 2017

aidanhs commented Jun 7, 2017

bors commented Jun 8, 2017

bors commented Jun 8, 2017

hanna-kruppe Mar 21, 2017 •

edited

Loading

steveklabnik commented Mar 23, 2017 •

edited

Loading

irfanhudda commented Mar 23, 2017 •

edited

Loading

irfanhudda commented Mar 23, 2017 •

edited

Loading

Mark-Simulacrum commented Jun 7, 2017 •

edited

Loading