Skip to content

unchecked_{shl|shr} should use u32 as the RHS - #103456

Merged
bors merged 2 commits into
rust-lang:masterfrom
scottmcm:fix-unchecked-shifts
Nov 19, 2022
Merged

unchecked_{shl|shr} should use u32 as the RHS#103456
bors merged 2 commits into
rust-lang:masterfrom
scottmcm:fix-unchecked-shifts

Conversation

@scottmcm

Copy link
Copy Markdown
Member

The other shift methods, such as https://doc.rust-lang.org/nightly/std/primitive.u64.html#method.checked_shr and https://doc.rust-lang.org/nightly/std/primitive.i16.html#method.wrapping_shl, use u32 for the shift amount. That's consistent with other things, like count_ones, which also always use u32 for a bit count, regardless of the size of the type.

This PR changes unchecked_shl and unchecked_shr to also use u32 for the shift amount (rather than Self).

cc #85122, the unchecked_math tracking issue

@rustbotrustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Oct 24, 2022
@rust-highfive

Copy link
Copy Markdown
Contributor

r? @joshtriplett

(rust-highfive has picked a reviewer for you, use r? to override)

@rustbot

Copy link
Copy Markdown
Collaborator

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

  • Stabilizing library features
  • Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
  • Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
  • Changing public documentation in ways that create new stability guarantees
  • Changing observable runtime behavior of library APIs

@rust-highfiverust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 24, 2022
Comment threadsrc/test/codegen/unchecked_shifts.rs Outdated

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

annot: the LLVM instruction always gives poison by default for too-high shift amounts (https://llvm.org/docs/LangRef.html#shl-instruction), so no extra flags are expected in the IR.

Comment threadlibrary/core/src/num/int_macros.rs Outdated

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

annot: this was previously using the intrinsic on stable, so the rustc_allow_const_fn_unstable isn't exposing anything new to stable here.

Comment threadlibrary/core/src/num/int_macros.rs Outdated

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

annot: the .ok() looks silly here, but Result::unwrap_unchecked isn't const, whereas Option::unwrap_uncheckedisconst.

@scottmcm
scottmcmforce-pushed the fix-unchecked-shifts branch from debadbc to 3b16c04CompareOctober 24, 2022 00:33
@scottmcmscottmcm added T-libs-api [DEPRECATED; DO NOT USE] and removed T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Oct 25, 2022
@scottmcmscottmcm added the I-libs-api-nominated [DEPRECATED; DO NOT USE] label Nov 15, 2022
@scottmcm

Copy link
Copy Markdown
MemberAuthor

Hi libs-api! I'm nominating this one to get a 👍/👎 on the signature change, in hopes of then being able to move it to a less-busy reviewer once the API decision is made.

@m-ou-se

Copy link
Copy Markdown
Member

@bors r+

@bors

bors commented Nov 15, 2022

Copy link
Copy Markdown
Collaborator

📌 Commit 3b16c04 has been approved by m-ou-se

It is now in the queue for this repository.

@borsbors 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 Nov 15, 2022
@m-ou-sem-ou-se removed the I-libs-api-nominated [DEPRECATED; DO NOT USE] label Nov 15, 2022
@scottmcmscottmcm assigned m-ou-se and unassigned joshtriplettNov 16, 2022
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Nov 16, 2022
…-ou-se
`unchecked_{shl|shr}` should use `u32` as the RHS
The other shift methods, such as https://doc.rust-lang.org/nightly/std/primitive.u64.html#method.checked_shr and https://doc.rust-lang.org/nightly/std/primitive.i16.html#method.wrapping_shl, use `u32` for the shift amount. That's consistent with other things, like `count_ones`, which also always use `u32` for a bit count, regardless of the size of the type.
This PR changes `unchecked_shl` and `unchecked_shr` to also use `u32` for the shift amount (rather than Self).
cc rust-lang#85122, the `unchecked_math` tracking issue
@Dylan-DPCDylan-DPC mentioned this pull request Nov 16, 2022
@scottmcm

scottmcm commented Nov 16, 2022

Copy link
Copy Markdown
MemberAuthor

@bors rollup=iffy (failed in a rollup once, but with the update that config worked in CI)
@bors r-

Failed in rollup https://github.com/rust-lang-ci/rust/actions/runs/3477142652/jobs/5813017785

@borsbors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 16, 2022
@rustbotrustbot added the A-testsuite Area: The testsuite used to check the correctness of rustc label Nov 17, 2022
@rustbotrustbot added the T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. label Nov 17, 2022
@scottmcm
scottmcmforce-pushed the fix-unchecked-shifts branch 2 times, most recently from 1e783ac to 9d4b1f9CompareNovember 17, 2022 00:44
@scottmcm

Copy link
Copy Markdown
MemberAuthor

Did the test-in-CI trick, and it doesn't fail that config any more: https://github.com/rust-lang/rust/actions/runs/3484161910/jobs/5828408349

@bors r+

@bors

bors commented Nov 17, 2022

Copy link
Copy Markdown
Collaborator

📌 Commit 9d4b1f9 has been approved by scottmcm

It is now in the queue for this repository.

@bors

bors commented Nov 17, 2022

Copy link
Copy Markdown
Collaborator

🌲 The tree is currently closed for pull requests below priority 1. This pull request will be tested once the tree is reopened.

@borsbors 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 17, 2022
Manishearth added a commit to Manishearth/rust that referenced this pull request Nov 18, 2022
…cottmcm
`unchecked_{shl|shr}` should use `u32` as the RHS
The other shift methods, such as https://doc.rust-lang.org/nightly/std/primitive.u64.html#method.checked_shr and https://doc.rust-lang.org/nightly/std/primitive.i16.html#method.wrapping_shl, use `u32` for the shift amount. That's consistent with other things, like `count_ones`, which also always use `u32` for a bit count, regardless of the size of the type.
This PR changes `unchecked_shl` and `unchecked_shr` to also use `u32` for the shift amount (rather than Self).
cc rust-lang#85122, the `unchecked_math` tracking issue
Manishearth added a commit to Manishearth/rust that referenced this pull request Nov 18, 2022
…cottmcm
`unchecked_{shl|shr}` should use `u32` as the RHS
The other shift methods, such as https://doc.rust-lang.org/nightly/std/primitive.u64.html#method.checked_shr and https://doc.rust-lang.org/nightly/std/primitive.i16.html#method.wrapping_shl, use `u32` for the shift amount. That's consistent with other things, like `count_ones`, which also always use `u32` for a bit count, regardless of the size of the type.
This PR changes `unchecked_shl` and `unchecked_shr` to also use `u32` for the shift amount (rather than Self).
cc rust-lang#85122, the `unchecked_math` tracking issue
This was referenced Nov 18, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 18, 2022
…earth
Rollup of 8 pull requests
Successful merges:
- rust-lang#102977 (remove HRTB from `[T]::is_sorted_by{,_key}`)
- rust-lang#103378 (Fix mod_inv termination for the last iteration)
- rust-lang#103456 (`unchecked_{shl|shr}` should use `u32` as the RHS)
- rust-lang#103701 (Simplify some pointer method implementations)
- rust-lang#104047 (Diagnostics `icu4x` based list formatting.)
- rust-lang#104338 (Enforce that `dyn*` coercions are actually pointer-sized)
- rust-lang#104498 (Edit docs for `rustc_errors::Handler::stash_diagnostic`)
- rust-lang#104556 (rustdoc: use `code-header` class to format enum variants)
Failed merges:
r? `@ghost`
`@rustbot` modify labels: rollup
@bors
bors merged commit e230115 into rust-lang:masterNov 19, 2022
@rustbotrustbot added this to the 1.67.0 milestone Nov 19, 2022
@scottmcm
scottmcm deleted the fix-unchecked-shifts branch November 19, 2022 03:31
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-testsuiteArea: The testsuite used to check the correctness of rustcS-waiting-on-borsStatus: Waiting on bors to run and complete tests. Bors will change the label on completion.T-infraRelevant to the infrastructure team, which will review and decide on the PR/issue.T-libs-api[DEPRECATED; DO NOT USE]

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@scottmcm@rust-highfive@rustbot@m-ou-se@bors@joshtriplett