-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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 RFC float-next-up-down. #3173
Conversation
The quoted API was a re-export from Java, which was already accounted for.
i think you could file a PR directly? https://github.com/rust-lang/rfcs/blob/master/libs_changes.md#is-an-rfc-required |
@kennytm I believe it falls under the 'new API' section, which suggests making an RFC. |
Whether new APIs require RFCs generally depends on surface area. Regardless, you've certainly put in the time to justify the change. |
I'm in favour of these two functions, for me their usage is rare but I've used them once or twice in other languages. The point of this RFC is to link to it from those function docs, to give complete documentation. (Regarding the names: as long as the documentation contains the common C function names, I think that next_float and prev_float are simpler to understand for common humans. Rust stdlib generally prefers more self-explanatory function names instead of the common and often more cryptic names. And I agree with this because remembering tens of strange function names isn't nice). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Much like the others, I feel like API addition of this scope does not necessarily warrant a full RFC process. Since we do have a nicely written RFC, perhaps consider making a PR with an implementation in parallel to this RFC as well? I suspect that it would be merged and made available to nightly users significantly before the RFC process concludes.
text/0000-float-next-up-down.md
Outdated
Two more functions get added to `f32` and `f64`, which may be considered | ||
already cluttered by some. | ||
|
||
Additionally, there is a minor pitfall regarding signed zero. Repeatedly calling |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This behaviour follows from the definition of the function, doesn't it? The definition says:
Returns the largest/smallest number more/less than
self
.
Since 0.0
and -0.0
compare equal, 0.0.next_down() == -0.0
would violate the contract.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, if we didn't want this we would need to change the definition of the function, and depart from IEEE 754 compliance. But I still felt I needed to mention this potential pitfall.
text/0000-float-next-up-down.md
Outdated
then `x` is supposed to be returned. Besides error signaling and NaNs, that is | ||
the complete specification. | ||
|
||
We did not choose this function for three reasons: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
next_up
and next_down
are also easier to implement more efficiently as they don't need to dispatch algorithm based on 2 inputs.
text/0000-float-next-up-down.md
Outdated
# Unresolved questions | ||
[unresolved-questions]: #unresolved-questions | ||
|
||
- Which is the better pair of names, `next_up` and `next_down` or `next_float` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_float
is already implied by the type in Rust.
Part of the reason I chose to come directly in full force is because
I could do that, I also have written tests, not part of the RFC. |
Wow, I have somehow never seen this doc and the last change to it was 6 years ago... I'm gonna go ahead and create a zulip thread about deduplicating that info... But as everyone else said, this change certainly does not require an RFC. The most up to date documentation on when an RFC is necessary is documented here: https://github.com/rust-lang/governance/blob/master/teams/libs/subteam-api.md#making-changes-to-the-standard-libraries. Either way though the RFC is appreciated and certainly doesn't hurt. |
@rfcbot merge Also, I want to note that I've already approved the PR adding the methods to std since they're unstable. I figure if we decide to not merge the RFC we can always remove them. |
Team member @yaahc has proposed to merge this. The next step is review by the rest of the tagged team members: No concerns currently listed. Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for info about what commands tagged team members can give me. |
I like following IEEE for the names and semantics. I don't think we're likely to find a name that's better enough to be worth diverging from that. I've done the hacky incorrect version of this multiple times ( |
Added next_up and next_down for f32/f64. This is a pull request implementing the features described at rust-lang/rfcs#3173.
Added next_up and next_down for f32/f64. This is a pull request implementing the features described at rust-lang/rfcs#3173.
🔔 This is now entering its final comment period, as per the review above. 🔔 |
The final comment period, with a disposition to merge, as per the review above, is now complete. As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed. This will be merged soon. |
Thank you! |
Huzzah! The @rust-lang/libs-api team has decided to accept this RFC. To track further discussion, subscribe to the tracking issue here: rust-lang/rust#91399 |
…ottmcm Added next_up and next_down for f32/f64. This is a pull request implementing the features described at rust-lang/rfcs#3173.
…ottmcm Added next_up and next_down for f32/f64. This is a pull request implementing the features described at rust-lang/rfcs#3173.
Add next_up and next_down for f32/f64 - take 2 This is a revival of rust-lang#88728 which staled due to inactivity of the original author. I've address the last review comment. --- This is a pull request implementing the features described at rust-lang/rfcs#3173. `@rustbot` label +T-libs-api -T-libs r? `@scottmcm` cc `@orlp`
Add next_up and next_down for f32/f64 - take 2 This is a revival of rust-lang/rust#88728 which staled due to inactivity of the original author. I've address the last review comment. --- This is a pull request implementing the features described at rust-lang/rfcs#3173. `@rustbot` label +T-libs-api -T-libs r? `@scottmcm` cc `@orlp`
Add next_up and next_down for f32/f64 - take 2 This is a revival of rust-lang/rust#88728 which staled due to inactivity of the original author. I've address the last review comment. --- This is a pull request implementing the features described at rust-lang/rfcs#3173. `@rustbot` label +T-libs-api -T-libs r? `@scottmcm` cc `@orlp`
Add next_up and next_down for f32/f64 - take 2 This is a revival of rust-lang/rust#88728 which staled due to inactivity of the original author. I've address the last review comment. --- This is a pull request implementing the features described at rust-lang/rfcs#3173. `@rustbot` label +T-libs-api -T-libs r? `@scottmcm` cc `@orlp`
This RFC adds two argumentless methods to
f32
/f64
,next_up
andnext_down
. These functions are specified in the IEEE 754 standard, and provide the capability to enumerate floating point values in order.Rendered.
There's also already an associated implementation pull request (rust-lang/rust#88728), should this RFC pass or be deemed unnecessary.