Update `wrapping_sh[lr]` docs and examples by scottmcm · Pull Request #149837 · rust-lang/rust (original) (raw)

Inspired by #general > `Source` link for `core` items is often inscrutable @ 💬 I wanted to add some more examples of the actual wrapping as well as update the documentation to emphasize that the behaviour is unusual.

In particular, now that unbounded_sh[lr] is stable, point people trying to avoid panics to that instead, since it behaves less weirdly.

Rendered example:

rustbot added S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

Relevant to the library team, which will review and decide on the PR/issue.

labels

r? @workingjubilee

rustbot has assigned @workingjubilee.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

#[doc = concat!("assert_eq!(42_", stringify!($SelfT), ".wrapping_shl(", stringify!($BITS), "), 42);")]
#[doc = concat!("assert_eq!(42_", stringify!($SelfT), ".wrapping_shl(1).wrapping_shl(", stringify!($BITS_MINUS_ONE), "), 0);")]
#[doc = concat!("assert_eq!((-1_", stringify!($SelfT), ").wrapping_shl(128), -1);")]
#[doc = concat!("assert_eq!(5_", stringify!($SelfT), ".wrapping_shl(1025), 10);")]

Choose a reason for hiding this comment

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

Might be nice to do some of these examples in binary (0b) or hex (0x), so one can see the bits move better

Choose a reason for hiding this comment

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

Yeah, actually, demonstrating the shift from 0b0101 to 0b1010 sounds nice. Don't have to do it for all of them, ofc.

Choose a reason for hiding this comment

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

Good call; I added a couple more basic examples to show the bits before the extreme examples.

Choose a reason for hiding this comment

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

/// then truncating as needed. The behaviour matches what shift instructions
/// do on many processors, and is what the `<<` operator does when overflow
/// checks are disabled, but numerically it's weird. Consider, instead,
/// using [`Self::unbounded_shl`] which has nicer behaviour.

Choose a reason for hiding this comment

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

Wait, hm.

Is "nicer" quite right? "More numerical" behavior? "As an integer"? "More logical"? "Is more similar to the mul-by-pow-2 operation you were looking for"?

Hmm, I think I've unsold myself on changing "nicer". We aren't that shy from opinion here, and docs contributors can wage that war as they see fit.

📌 Commit f9b830c has been approved by workingjubilee

It is now in the queue for this repository.

bors added S-waiting-on-bors

Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

and removed S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

labels

Sounds good; happy to leave "nicer" there for now :)

Yeah, "weird" and "nicer" are admittedly kinda vague. Hopefully the new examples elaborate sufficiently, since I couldn't find a succinct way to say something like "mean that large shifts work like you'd expect from doing a bunch of smaller shifts that add up".

(I'll ponder doing a follow-up PR to say more in unbounded_sh[lr] too.)

bors added a commit that referenced this pull request

Rollup of 8 pull requests

Successful merges:

#146794 (std: reorganize pipe implementations)
#148490 (dangling pointer from temp cleanup)
#149837 (Update wrapping_sh[lr] docs and examples)
#149864 (std: Don't use linkat on the wasm32-wasi* targets)
#149885 (replace addr_of_mut with &raw mut in maybeuninit docs)
#149949 (Cleanup of attribute parsing errors)
#149969 (don't use no_main and no_core to test IBT)
#149998 (miri subtree update)

r? @ghost @rustbot modify labels: rollup

bors added a commit that referenced this pull request

Rollup of 8 pull requests

Successful merges:

#146794 (std: reorganize pipe implementations)
#148490 (dangling pointer from temp cleanup)
#149837 (Update wrapping_sh[lr] docs and examples)
#149864 (std: Don't use linkat on the wasm32-wasi* targets)
#149885 (replace addr_of_mut with &raw mut in maybeuninit docs)
#149949 (Cleanup of attribute parsing errors)
#149969 (don't use no_main and no_core to test IBT)
#149998 (miri subtree update)

r? @ghost @rustbot modify labels: rollup

rust-timer added a commit that referenced this pull request