Add missing Wrapping methods, use doc_comment!

[clarfonthey]

Re-opened version of #49393 . Finishing touches for #32463.

Note that this adds Shl and Shr implementations for Wrapping<i128> and Wrapping<u128>, which were previously missed. This is technically insta-stable, but I don't know why this would be a problem.

[rust-highfive]

r? @KodrAus

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

[nagisa]

The implementation of wrapping_next_power_of_two is questionable.

Tne name and expectation from the other wrapping methods would make one to expect that when next_power_of_two would overflow, you’d get the smallest power of two again (i.e. 1), but this is not what would happen with the current implementation.

[clarfonthey]

@nagisa see the discussion in #49393 for more on that.

[nagisa]

There doesn’t seem to be any conclusion in that thread.

[nagisa]

To give an actual counter-argument to current behaviour of it wrapping to 0. There’s this intrinsic cttz_nonzero that is fairly useful when in combination of next_power_of_two. Now if we take the currently implemented wrapping behaviour, the code which assumes that "next power of two" follows the strict mathematical definition modulo 2⁶⁴/2³²/2¹⁶/... will sometimes result in undefined behaviour here.

[clarfonthey]

I'm a bit confused; wouldn't cttz_nonzero(x.next_power_of_two()) == BITS - ctlz_nonzero(x) when x is zero and 1 when x is nonzero?

Not 100% sure what you're trying to do here.

[rust-highfive]

The job x86_64-gnu-llvm-3.9 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

[00:02:52] Compiling core v0.0.0 (file:///checkout/src/libcore)
[00:02:52] Compiling build_helper v0.1.0 (file:///checkout/src/build_helper)
[00:02:52] Compiling unwind v0.0.0 (file:///checkout/src/libunwind)
[00:02:55] Compiling std v0.0.0 (file:///checkout/src/libstd)
[00:02:55] error[E0425]: cannot find value `u128` in module `self::shift_max`
[00:02:55] --> libcore/num/wrapping.rs:115:29
[00:02:55] |
[00:02:55] 115 | sh_impl_unsigned! { $t, usize }
[00:02:55] | ^^ not found in `self::shift_max`
[00:02:55] ...
[00:02:55] 125 | sh_impl_all! { u8 u16 u32 u64 u128 usize i8 i16 i32 i64 i128 isize }
[00:02:55] | -------------------------------------------------------------------- in this macro invocation
[00:02:55] [00:02:55] error[E0425]: cannot find value `i128` in module `self::shift_max`
[00:02:55] --> libcore/num/wrapping.rs:115:29
[00:02:55] |
[00:02:55] 115 | sh_impl_unsigned! { $t, usize }
[00:02:55] | ^^ not found in `self::shift_max`
[00:02:55] ...
[00:02:55] 125 | sh_impl_all! { u8 u16 u32 u64 u128 usize i8 i16 i32 i64 i128 isize }
[00:02:55] | -------------------------------------------------------------------- in this macro invocation
[00:02:59] Compiling compiler_builtins v0.0.0 (file:///checkout/src/rustc/compiler_builtins_shim)
[00:02:59] Compiling cmake v0.1.30
[00:02:59] Compiling alloc_jemalloc v0.0.0 (file:///checkout/src/liballoc_jemalloc)
[00:03:04] Compiling rustc_asan v0.0.0 (file:///checkout/src/librustc_asan)
---
[00:03:09] Caused by:
[00:03:09] process didn't exit successfully: `/checkout/obj/build/bootstrap/debug/rustc --crate-name core libcore/lib.rs --color always --error-format json --crate-type lib --emit=dep-info,link -C opt-level=3 -C metadata=fb1e36473ec4786e -C extra-filename=-fb1e36473ec4786e --out-dir /checkout/obj/build/x86_64-unknown-linux-gnu/stage0-std/x86_64-unknown-linux-gnu/release/deps --target x86_64-unknown-linux-gnu -L dependency=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-std/x86_64-unknown-linux-gnu/release/deps -L dependency=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-std/release/deps` (exit code: 101)
[00:03:09] warning: build failed, waiting for other jobs to finish...
[00:03:18] error: build failed
[00:03:18] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "build" "--target" "x86_64-unknown-linux-gnu" "--release" "--locked" "--color" "always" "--features" "panic-unwind jemalloc backtrace" "--manifest-path" "/checkout/src/libstd/Cargo.toml" "--message-format" "json"
[00:03:18] expected success, got: exit code: 101
[00:03:18] thread 'main' panicked at 'cargo must succeed', bootstrap/compile.rs:1091:9
[00:03:18] travis_fold:end:stage0-std
[00:03:18] travis_time:end:stage0-std:start=1525798039369893925,finish=1525798065220596060,duration=25850702135
[00:03:18] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:03:18] Build completed unsuccessfully in 0:00:27
[00:03:18] Makefile:79: recipe for target 'tidy' failed
[00:03:18] make: *** [tidy] Error 1
The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:0606d361
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

[nagisa]

@clarcharr rewriting such code in terms of ctlz is definitely possible, however it is harder to do if you want the "wrapping" power of two behaviour and the resulting code will be less obvious than the obvious solution that uses the wrapping power-of-two.

---

One definition that would support the current implementation would be

returns the next power of two greater than or equal to self modulo X.

Then doing a (2⁶³ + 1).next_power_of_two() = 2⁶⁴ mod 2⁶⁴ = 0.

But in that case we must, in very clear manner warn users that this doesn’t always result in an actual power-of-two being returned.

[clarfonthey]

That is fair, and I'll update the documentation for it. Also as a precaution, I'll give it a different feature flag to make it clear that it should be discussed more in depth during stabilisation.

[emilyalbini]

Ping from triage! What's the status of this PR?

[clarfonthey]

@pietroalbini I still need to separate out the wrapping_next_power_of_two methods into a separate feature, but otherwise, this is ready for review.

IMHO merging the next power of two functions now is okay because they're unstable, but a separate feature flag should mention that the behaviour might change for reasons @nagisa mentioned.

[clarfonthey]

Updated those methods with a separate feature flag, also added a reason field explaining that the wrapping behaviour may change.

[rust-highfive]

The job x86_64-gnu-llvm-3.9 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

[00:05:13] travis_fold:start:tidy
travis_time:start:tidy
tidy check
[00:05:13] tidy error: /checkout/src/libcore/num/mod.rs:3443: line longer than 100 chars
[00:05:13] tidy error: /checkout/src/libcore/num/wrapping.rs:885: line longer than 100 chars
[00:05:15] some tidy checks failed
[00:05:15] [00:05:15] [00:05:15] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor" "--quiet"
[00:05:15] [00:05:15] [00:05:15] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:05:15] Build completed unsuccessfully in 0:01:56
[00:05:15] Build completed unsuccessfully in 0:01:56
[00:05:15] Makefile:79: recipe for target 'tidy' failed
[00:05:15] make: *** [tidy] Error 1
The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:154508ee
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

[emilyalbini]

Ping from triage @KodrAus! This PR needs your review.

[KodrAus]

Thanks @clarcharr!

So looking back through the history it looks like we haven't quite got a concrete use-case to base the wrapping power of two methods on but have at least reached a point where their behaviour is well communicated so we could add a note to the tracking issue to look at them again before stabilization. So that seems good to me.

@clarcharr Sorry, where are we implementing Shl and Shr for Wrapping now?

[clarfonthey]

@KodrAus Right now the Shl and Shr impls only allow usize on the right-hand-side. This simply adds impls of that for Wrapping<i128> and Wrapping<u128> which were missed by accident, and necessary to make all of the doc tests pass here.

[KodrAus]

Ah gotcha 👍 That shouldn't be a problem for anyone. This looks good to me, I'll add a note to the tracking issue. Thanks @clarcharr!

@bors r+

[bors]

📌 Commit 99cf5a9 has been approved by KodrAus

[bors]

⌛ Testing commit 99cf5a9 with merge e9a489b...

[bors]

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