-
Notifications
You must be signed in to change notification settings - Fork 6.2k
8367942: Add API note discussing Double.compareTo total order and IEEE 754 total order #27356
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
Conversation
… IEEE 754 total order
|
👋 Welcome back darcy! A progress list of the required criteria for merging this PR into |
|
@jddarcy This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 75 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
Webrevs
|
| * particular IEEE 754 regards "negative" NaN representations, | ||
| * that is NaN representations whose sign bit is set, to be less | ||
| * than any finite or infinite value, including negative | ||
| * infinity. Also in the IEEE 754 ordering, "positive" NaN values |
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 last sentence seems redundant because the behavior for positive NaN values are the same for both total orders.
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.
Hmm. Yeah, I wanted to avoid giving too much details on what IEEE 754 does here; they also ascribe ordering between different NaN bit patterns while Double.compareTo puts all NaNs into the same equivalence class. I'll take another run at the text; thanks.
|
The leading words of the PR description are not in total order 😉 |
|
The note looks good to me. |
|
Looks all right to me once the placeholder text is replaced. |
| * value and less than any "positive" NaN. In addition, the IEEE | ||
| * order regards all positive NaN values as greater than positive | ||
| * infinity. | ||
| * |
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.
When both arguments are NaNs, IEEE 754 also specifies (§5.10.d.5.ii)
signaling orders below quiet for +NaN, reverse for −NaN
and leaves more fine grained details to the implementation.
Otherwise looks fine.
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.
When both arguments are NaNs, IEEE 754 also specifies (§5.10.d.5.ii)
signaling orders below quiet for +NaN, reverse for −NaN
and leaves more fine grained details to the implementation.
Otherwise looks fine.
Hmm. I didn't necessarily want to given all the details of the IEEE 754 total order here, only note that it differs from the total order used in the Java SE API. For example, the entirety of the Java specifications avoid mentioning or explaining the differences between quiet and signaling NaNs and I'd prefer not to get into that here.
I think adding a blanket statement "see the IEEE 754 standard for full detail on their total order" would satisfy any lingering uncertainty here. What do you think?
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.
Sure, that would clarify any doubt.
The API note mentions negative NaNs, which are not part of the Java spec either. So I came to the conclusion that differentiating between the various NaN categories in IEEE 754 was part of the intent of the note.
Placeholder text now replaced. |
|
/integrate |
|
Going to push as commit 4882559.
Your commit was automatically rebased without conflicts. |
Add a total that the total order used by {Double, Float}.compareTo is different than the total order defined by IEEE 754, starting the 2008 version of that standard.
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/27356/head:pull/27356$ git checkout pull/27356Update a local copy of the PR:
$ git checkout pull/27356$ git pull https://git.openjdk.org/jdk.git pull/27356/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 27356View PR using the GUI difftool:
$ git pr show -t 27356Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/27356.diff
Using Webrev
Link to Webrev Comment