JDK-8257086: Clarify differences between {Float, Double}.equals and ==#1699
JDK-8257086: Clarify differences between {Float, Double}.equals and ==#1699jddarcy wants to merge 10 commits intoopenjdk:masterfrom
Conversation
|
👋 Welcome back darcy! A progress list of the required criteria for merging this PR into |
Webrevs
|
| * | ||
| * <p>also has the value {@code true}. However, there are two | ||
| * exceptions: | ||
| * exceptions where the properties of an equivalence relations are |
|
|
||
| * This method imposes a total order on {@code Double} objects | ||
| * with two differences compared to the incomplete order defined the | ||
| * by Java language numerical comparison operators ({@code <, <=, |
There was a problem hiding this comment.
Typo: defined the by Java -> defined by the Java.
|
I'll note initially that the original bug is about While this material is covering the right ground, I'd say that it's too verbose for a method specification. It feels like it's being compressed to fit into a method specification and thus doesn't do the topic justice. (One additional concept that ought to be covered is that I'll suggest a couple alternatives. One is to put a full, combined explanation into class doc somewhere, say in Another alternative is to put this explanation into the |
I added discussion of compareTo as well as equals to the scope of the bug since they are sibling surprising deviations from what is expected and have the same root cause. (I took care to say "incomplete order" rather than "partial order" since a partial order requires reflexivity.) The interface java.lang.Comparable gives a definition of "consistent with equals." I didn't verify it doesn't have an anchor to link to, but we don't typically link to the definition. However, I wouldn't be adverse to having "consistent with equals" link to Comparable; that would be more obvious than expecting the reader to follow the "Specified by: compareTo in interface Comparable" link javadoc generates for the method. I think the Float and Double equals and compareTo methods should at least get pointers to any new section added -- I think a standalone section in the package javadoc by itself would have low discoverability. I'm make another iteration over the text; thanks. |
|
Great, let me know if you'd like me to provide some text for any particular topics in this area. |
|
@jddarcy This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration! |
Before sending out another iteration in code, there the draft javadoc text of a discussion at the end of the class-level discussion of java.lang.Double:
Comments? Thanks, |
|
Mailing list message from Bernd Eckenfels on core-libs-dev: I like the text it?s good to mix object and value identities. I would only miss unequal behavior of NaN in the description. Gruss -- On Thu, 10 Dec 2020 01:32:24 GMT, Stuart Marks <smarks at openjdk.org> wrote:
Before sending out another iteration in code, there the draft javadoc text of a discussion at the end of the class-level discussion of java.lang.Double: * <h2><a id=equivalenceRelation>Floating-point Equality, Equivalence, Comments? Thanks, ------------- PR: https://git.openjdk.java.net/jdk/pull/1699 |
|
I took the liberty of making an editing pass over the proposed text. Along with a few editorial and markup adjustments, the key changes are as follows:
We'll see how well this survives github markup. If it doesn't work out, I can email the text file. (Triple backquotes seem to do the trick.) |
|
Mailing list message from Joe Darcy on core-libs-dev: Hi Stuart, On 1/28/2021 7:19 PM, Stuart Marks wrote:
[snip]
Yes; substitutability was an implicit concept in the earlier write-up; I'll work on integrating this version into an update of the PR with Thanks, -Joe |
stuart-marks
left a comment
There was a problem hiding this comment.
Overall good! Just some whitespace errors and a couple typos as noted.
| * comparison</i> does <em>not</em> hold. | ||
| * | ||
| * <p>To provide the appropriate semantics for {@code equals} and {@code | ||
| * compareTo} methods, those methods cannot simply to wrappers around |
There was a problem hiding this comment.
I think this should be "cannot simply be wrappers".
|
@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 16 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 |
stuart-marks
left a comment
There was a problem hiding this comment.
I did another full reread. Everything looks good!
|
/reviewer credit bpb |
|
@jddarcy |
|
/integrate |
|
@jddarcy Since your change was applied there have been 16 commits pushed to the
Your commit was automatically rebased without conflicts. Pushed as commit 474dba2. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Updates to the specifications of Double.{equals, compareTo} to explain more explicitly why the obvious wrappers around "==" and "<"/"==" are not sufficient for floating-point values.
Once the wording is worked out, I'll replicate it for the analogous methods on Float.
Progress
Issue
Reviewers
Download
$ git fetch https://git.openjdk.java.net/jdk pull/1699/head:pull/1699$ git checkout pull/1699