Skip to content

Add isqrt to NonZero<uN>#126199

Merged
bors merged 2 commits intorust-lang:masterfrom
ivan-shrimp:nonzero_isqrt
Jul 19, 2024
Merged

Add isqrt to NonZero<uN>#126199
bors merged 2 commits intorust-lang:masterfrom
ivan-shrimp:nonzero_isqrt

Conversation

@ivan-shrimp
Copy link
Contributor

Implements #70887 (comment), with the following signature:

impl NonZero<uN> {
    const fn isqrt(self) -> Self;
}

Unintended benefits include one fewer panicking branch in ilog2 for LLVM to optimize away, and one fewer assume_unchecked as NonZero already does that.

The fast path for self == 1 is dropped, but the current implementation is very slow anyways compared to hardware. Performance improvements can always come later.

(I didn't add the function to NonZero<iN>, since every existing NonZero method is non-panicking, and it might be nice to leave it that way.)

@rustbot
Copy link
Collaborator

rustbot commented Jun 9, 2024

r? @Nilstrieb

rustbot has assigned @Nilstrieb.
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

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jun 9, 2024
@rust-log-analyzer

This comment has been minimized.

@Noratrieb
Copy link
Member

r? libs-api

@rustbot rustbot added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Jun 9, 2024
@rustbot rustbot assigned BurntSushi and unassigned Noratrieb Jun 9, 2024
@Noratrieb
Copy link
Member

I recommend you open an API Change Proposal as described here, it's a more efficient way to get the libs-api team to decide on whether this should be accepted or not: https://std-dev-guide.rust-lang.org/development/feature-lifecycle.html
Use @rustbot ready when your ACP has been accepted.
@rustbot author
@rustbot label S-waiting-on-ACP

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. S-waiting-on-ACP Status: PR has an ACP and is waiting for the ACP to complete. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 9, 2024
@ivan-shrimp
Copy link
Contributor Author

The original isqrt PR landed without an ACP, and this is probably just a straightforward extension. Nonetheless, I started an ACP and I'll wait for it to be accepted.

@tgross35
Copy link
Contributor

ACP accepted rust-lang/libs-team#391 (comment)

@rustbot ready

@rustbot rustbot 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 Jun 11, 2024
@ivan-shrimp
Copy link
Contributor Author

@rustbot label -S-waiting-on-ACP

@rustbot rustbot removed the S-waiting-on-ACP Status: PR has an ACP and is waiting for the ACP to complete. label Jun 12, 2024
pub const fn checked_ilog2(self) -> Option<u32> {
// FIXME: Simply use `NonZero::new` once it is actually generic.
if let Some(x) = <$NonZeroT>::new(self) {
if let Some(x) = NonZero::new(self) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe change these to match rather than if let, to be consistent with the new isqrt? Or even .map(NonZero::<Self>::ilog10).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Option::map is not const (not even unstably). match does look a bit nicer though.

Comment on lines +1253 to +1275
unsafe {
hint::assert_unchecked(res < 1 << (Self::BITS / 2));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
unsafe {
hint::assert_unchecked(res < 1 << (Self::BITS / 2));
}
unsafe { hint::assert_unchecked(res < 1 << (Self::BITS / 2)) };

Extremely minor nit but semi usually looks nicer outside single-line braced blocks

@ivan-shrimp
Copy link
Contributor Author

@rustbot author

@rustbot rustbot 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-review Status: Awaiting review from the assignee but also interested parties. labels Jun 16, 2024
@ivan-shrimp
Copy link
Contributor Author

@rustbot ready

@rustbot rustbot 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 Jun 16, 2024
@bors
Copy link
Collaborator

bors commented Jun 24, 2024

☔ The latest upstream changes (presumably #126914) made this pull request unmergeable. Please resolve the merge conflicts.

@rustbot rustbot removed O-ios Operating system: iOS O-itron Operating System: ITRON O-linux Operating system: Linux O-macos Operating system: macOS O-SGX Target: SGX O-solid Operating System: SOLID O-unix Operating system: Unix-like labels Jul 19, 2024
@tgross35 tgross35 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed O-windows Operating system: Windows T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) O-wasm Target: WASM (WebAssembly), http://webassembly.org/ S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. T-release Relevant to the release subteam, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. T-style Relevant to the style team, which will review and decide on the PR/issue. PG-exploit-mitigations Project group: Exploit mitigations WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver) O-wasi Operating system: Wasi, Webassembly System Interface labels Jul 19, 2024
@tgross35
Copy link
Contributor

libs-api already discussed this and it only modifies unstable API, so I think this is okay to merge.

Thanks for the addition!

r? @tgross35
@bors r+ rollup

@bors
Copy link
Collaborator

bors commented Jul 19, 2024

📌 Commit 90bba8b has been approved by tgross35

It is now in the queue for this repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants