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

Added next_up and next_down for f32/f64. #88728

Closed
wants to merge 8 commits into from

Conversation

orlp
Copy link
Contributor

@orlp orlp commented Sep 7, 2021

This is a pull request implementing the features described at rust-lang/rfcs#3173.

@rust-highfive
Copy link
Collaborator

r? @yaahc

(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 Sep 7, 2021
@kpreid
Copy link
Contributor

kpreid commented Sep 7, 2021

In the RFC you mention “Repeatedly calling next_up on a negative number will iterate over all values above it, with the exception of +0.0, only -0.0 will be visited.” I think this point would be good to make explicitly in the documentation of these functions — it is a place where the two goals “exhaust the space of float-representable numbers” and “move along the number line in the smallest nonzero step” differ, and it's worth mentioning this to, if nothing else, prompt the programmer to think about which thing they're doing and whether the function's inherent behavior is correct for their use case.

@the8472 the8472 added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Sep 8, 2021
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 27, 2021
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 19, 2021
@scottmcm
Copy link
Member

scottmcm commented Oct 21, 2021

👍 to having these. I think we should explicitly mention that these are expected to match IEEE's nextUp and nextDown operations, and keep the next_up and next_down names.

I don't think we'll find any name that's substantially better, so sticking with precedent seems best.

(And we can add another thing with different names if there ends up being a desire to have something that doesn't follow IEEE exactly, like if there were one that visited every bit sequence.)

@yaahc
Copy link
Member

yaahc commented Oct 22, 2021

Everything looks good and I don't think I have anything to add other than to say thank you for the wonderful PR! I'm gonna go ahead and merge this since it's all nightly only and then I'll start an FCP on the RFC since it's already there, might as well not waste it.

@bors r+

@bors
Copy link
Contributor

bors commented Oct 22, 2021

📌 Commit 4dd9b85 has been approved by yaahc

@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 Oct 22, 2021
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Oct 22, 2021
Added next_up and next_down for f32/f64.

This is a pull request implementing the features described at rust-lang/rfcs#3173.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Oct 22, 2021
Added next_up and next_down for f32/f64.

This is a pull request implementing the features described at rust-lang/rfcs#3173.
@matthiaskrgr
Copy link
Member

Some test were failing in a rollup, looks like it was caused by this PR: #90178 (comment)

@bors r-

@bors bors 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 Oct 23, 2021
@bors
Copy link
Contributor

bors commented Oct 26, 2021

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

@orlp
Copy link
Contributor Author

orlp commented Oct 30, 2021

So investigating why my pull request failed, I'm not sure that it's my fault. This fails on i586:

f32::from_bits(0x7F955555).to_bits() == 0x7F955555

This violates the standard library guarantees, which say that from_bits and to_bits are transmutes.

It's not my code that failed either way, it's just that the way I wrote the testcase assumes from_bits and to_bits are sane and do not magically change my bits under the hood:

let nan0 = f32::NAN.to_bits();
let nan1 = f32::NAN.to_bits() ^ 0x002a_aaaa;
let nan2 = f32::NAN.to_bits() ^ 0x0055_5555;
assert_eq!(f32::from_bits(nan0).next_down().to_bits(), nan0);
assert_eq!(f32::from_bits(nan1).next_down().to_bits(), nan1);
assert_eq!(f32::from_bits(nan2).next_down().to_bits(), nan2);

@scottmcm
Copy link
Member

NANs are always a bit weird, and pre-i686 floats are particularly annoying. You might be interested in the conversations in #73328

But honestly for now I'd just say cfg out that test on i586.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 16, 2021
…ottmcm

Added next_up and next_down for f32/f64.

This is a pull request implementing the features described at rust-lang/rfcs#3173.
@matthiaskrgr
Copy link
Member

Still failing in ci I guess: #91983 (comment)

@bors rollup=never r-

@bors bors 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 Dec 16, 2021
@orlp
Copy link
Contributor Author

orlp commented Dec 16, 2021

I don't know how it was marked to be ready to merged since we knew it was failing two weeks ago, sorry for wasting your time.

The problem is that x87 floating point is very problematic for this set of functions. At first I thought it was just NaN bits not being preserved, but it appears that it's also flushing denormals to zero, not just when doing arithmetic operations, but also when passing values through function boundaries.

I think I'll disable testing on x87 floating point platforms for now and indeed add an unresolved question to the tracking issue. I honestly don't know what to do with this platform.

@matthiaskrgr
Copy link
Member

Oh strange, you're right I didn't see that.
Sometimes bors acts funny and the queue shows prs that were already unapproved as approved, guess that was one of those times.
Sorry for the noise... 😄

/// - if `self` is [`MAX`] or [`INFINITY`], this returns [`INFINITY`];
/// - otherwise the unique least value greater than `self` is returned.
///
/// The identity `x.next_up() == -(-x).next_down()` holds for all `x`. When `x`
Copy link
Contributor

Choose a reason for hiding this comment

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

This is technically not true for x = NAN.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why not?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I think I see what you're getting at, == here is to be understood as a bitwise / definition equality, not floating point comparison where NaN != NaN.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I update this doc comment to say "for all non-NaN x"? Or maybe reword it to "x.next_up() is equivalent to -(-x).next_down() for all x" to prevent confusion with == failing for NaNs?

@JohnCSimon
Copy link
Member

ping from triage:
@orlp Can you address the review comments and resolve the conversation? we have some build failures, too.

When it's ready for review send a message containing
@rustbot ready to switch to S-waiting-on-review

@Dylan-DPC
Copy link
Member

@orlp any updates on this?

@orlp
Copy link
Contributor Author

orlp commented Feb 19, 2022

@Dylan-DPC I think I've been procrastinating on this because I don't see a good solution. Today I'll update the tracking issue with the unresolved question on what to do with x87, disable the testing for that platform for now and push my changes.

@orlp
Copy link
Contributor Author

orlp commented Feb 19, 2022

I've posted the unresolved question to the tracking issue: #91399 (comment)

Should I wait with asking a review for this until the unresolved question is resolved or do it now @JohnCSimon ?

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Mar 5, 2022

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

@JohnCSimon JohnCSimon 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 11, 2022
@JohnCSimon
Copy link
Member

@orlp - if you think it's ready for review, go ahead and mark it ready for review. I'm just doing PR triage.

@JohnCSimon
Copy link
Member

@orlp
Ping from triage: I'm closing this due to inactivity, Please reopen when you are ready to continue with this.
Note: if you do please open the PR BEFORE you push to it, else you won't be able to reopen - this is a quirk of github.
Thanks for your contribution.

@rustbot label: +S-inactive

@JohnCSimon JohnCSimon closed this Jun 19, 2022
@rustbot rustbot added the S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. label Jun 19, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 28, 2022
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`
workingjubilee pushed a commit to tcdi/postgrestd that referenced this pull request Sep 15, 2022
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`
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this pull request Apr 20, 2024
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`
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this pull request Apr 27, 2024
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`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. 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.