Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add unchecked math inherant impls to integers #63923

Closed
wants to merge 3 commits into from

Conversation

CAD97
Copy link
Contributor

@CAD97 CAD97 commented Aug 26, 2019

This exposes the unchecked_add and unchecked_sub intrinsics on integers.

If we agree we want this, I'll make a tracking issue and point the #[unstable] attributes at it.

This would be used by #62886 at a minimum.

@rust-highfive
Copy link
Collaborator

r? @joshtriplett

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 26, 2019
@Centril
Copy link
Contributor

Centril commented Aug 26, 2019

N.B. the discussion in #59148. cc @rkruppe @ubsan

@CAD97
Copy link
Contributor Author

CAD97 commented Aug 26, 2019

Honestly, I'd be fine having these be feature = "rustc_internals", issue = "0" forever; however, it's a lot simpler to use them where they're actually beneficial as methods (or functions) that are actually typed rather than the intrinsics.

They'd be less easy to reach for if they weren't available as methods and had to be used as e.g. usize::unchecked_add, if that would make it easier to swallow.

@Centril
Copy link
Contributor

Centril commented Aug 26, 2019

Honestly, I'd be fine having these be feature = "rustc_internals", issue = "0" forever; however, it's a lot simpler to use them where they're actually beneficial as methods (or functions) that are actually typed rather than the intrinsics.

As long as we're using issue = "0" I think it won't need sign-off from any team and then you also don't need to create a tracking issue. I think that's a good idea if your goal is simply to use these internally though it might be preferable if you cherry-pick this PR into the other one + perf numbers to show that the added unsafe is actually beneficial.

@joshtriplett
Copy link
Member

What's the purpose and use case for these functions, as opposed to functions guaranteed to have wrapping semantics?

@strega-nil
Copy link
Contributor

@joshtriplett better optimization -- see the original PR to add the intrinsics.

@JohnCSimon
Copy link
Member

Ping from triage
@CAD97 @joshtriplett This has sat idle for nearly a week. I'm not sure what else needs to happen for this PR?

Thanks.

@CAD97
Copy link
Contributor Author

CAD97 commented Sep 8, 2019

I'm going to close this and include it in the use case for now as an issue = "0" stdlib-internal interface.

@CAD97 CAD97 closed this Sep 8, 2019
@CAD97 CAD97 deleted the patch-1 branch October 1, 2019 17:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants