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

Stabilize float::to_int_unchecked #70487

Merged
merged 1 commit into from Apr 3, 2020

Conversation

Mark-Simulacrum
Copy link
Member

@Mark-Simulacrum Mark-Simulacrum commented Mar 27, 2020

This renames and stabilizes unsafe floating point to integer casts, which are intended to be the substitute for the currently unsound as behavior, once that changes to safe-but-slower saturating casts. As such, I believe this also likely unblocks #10184 (our oldest I-unsound issue!), as once this rolls out to stable it would be far easier IMO to change the behavior of as to be safe by default.

This does not stabilize the trait or the associated method, as they are deemed internal implementation details (and consumers should not, generally, want to expose them, as in practice all callers likely know statically/without generics what the return type is).

Closes #67058

@rust-highfive
Copy link
Collaborator

r? @cramertj

(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 Mar 27, 2020
@Mark-Simulacrum
Copy link
Member Author

I am uncertain if we want another round of FCP, but I think likely no. To confirm, r? @SimonSapin

@Amanieu
Copy link
Member

Amanieu commented Mar 27, 2020

I think we're just waiting on the FCP in #67058.

@Centril Centril added this to the 1.44 milestone Mar 28, 2020
@Centril Centril added relnotes Marks issues that should be documented in the release notes of the next release. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Mar 28, 2020
@SimonSapin
Copy link
Contributor

I’d prefer if the trait method name and the intrinsic name were kept consistent (perhaps with float_ prepended for the intrinsic)

@Mark-Simulacrum Mark-Simulacrum force-pushed the float-unchecked-casts branch 2 times, most recently from 1a761df to 9947a4e Compare March 28, 2020 16:00
@Mark-Simulacrum
Copy link
Member Author

Updated the intrinsic to float_to_int_unchecked as well.

Copy link
Contributor

@SimonSapin SimonSapin left a comment

Choose a reason for hiding this comment

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

r=me when the FCP finishes

@bors
Copy link
Contributor

bors commented Mar 29, 2020

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

This renames and stabilizes unsafe floating point to integer casts, which are
intended to be the substitute for the currently unsound `as` behavior, once that
changes to safe-but-slower saturating casts.
@SimonSapin
Copy link
Contributor

@bors r+ #67058 (comment)

@bors
Copy link
Contributor

bors commented Apr 2, 2020

📌 Commit 5614721 has been approved by SimonSapin

@bors 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 Apr 2, 2020
Centril added a commit to Centril/rust that referenced this pull request Apr 2, 2020
…ts, r=SimonSapin

Stabilize float::to_int_unchecked

This renames and stabilizes unsafe floating point to integer casts, which are intended to be the substitute for the currently unsound `as` behavior, once that changes to safe-but-slower saturating casts. As such, I believe this also likely unblocks rust-lang#10184 (our oldest I-unsound issue!), as once this rolls out to stable it would be far easier IMO to change the behavior of `as` to be safe by default.

This does not stabilize the trait or the associated method, as they are deemed internal implementation details (and consumers should not, generally, want to expose them, as in practice all callers likely know statically/without generics what the return type is).

Closes rust-lang#67058
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 2, 2020
Rollup of 7 pull requests

Successful merges:

 - rust-lang#70487 (Stabilize float::to_int_unchecked)
 - rust-lang#70595 (Remove unused discriminant reads from MIR bodies)
 - rust-lang#70691 (Improve docs in `AllocRef`)
 - rust-lang#70694 (Use Self over specific type in return position)
 - rust-lang#70700 (Expand on platform details of `include_xxx` macros)
 - rust-lang#70708 (Fix typo in u8::to_ascii_uppercase and u8::to_ascii_lowercase)
 - rust-lang#70716 (Unerase regions in infer_placeholder_type)

Failed merges:

r? @ghost
@bors bors merged commit 1eabbd0 into rust-lang:master Apr 3, 2020
@CodesInChaos
Copy link

I would have preferred having the word trunc(ate) in the method name, but it looks like it's too late to change that now.

@SimonSapin
Copy link
Contributor

@CodesInChaos In my opinion it depends what other conversion methods we end up adding to floats. In https://internals.rust-lang.org/t/pre-rfc-add-explicitly-named-numeric-conversion-apis/11395 (that I still need to finish writing up into a proper RFC) I proposed adding various conversion methods that could be preferred over as for their more explicit names and for including alternative semantics. For example the proposed (#10184) behavior for float-to-int as is saturation, but wrapping might make sense for some users.

Now with truncation here I assume you mean as opposed to other rounding modes such as round-to-nearest. I think having this be part of the name would be important if we eventually add something like round_to_int_unchecked. Could that be implemented in any way that’s significantly better than composing with the existing .round() method? If not, then I think it’s not worth having a dedicated method and we can instead suggest that pattern in docs. In that case, I don’t feel strongly about having the rounding mode in the method name.

If you do feel strongly, it’s not entirely too late. We have precedent for making changes to something after a stabilization PR has landed, before it reaches the Stable release channel. Consider filing a new issue (a merged PR is "resolved" and so not a great place for new discussion) with a concrete proposal (exact name) and some arguments. Not as formal as an RFC, but in the same spirit. @-mention me in it.

@CodesInChaos
Copy link

@SimonSapin I'd prefer the more descriptive name, but I don't feel strongly about this case, since truncation is the default in most languages and thus what people expect.

@Mark-Simulacrum Mark-Simulacrum deleted the float-unchecked-casts branch April 20, 2020 13:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
relnotes Marks issues that should be documented in the release notes of the next release. 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.

Tracking issue for {f32,f64}::to_int_unchecked methods
8 participants