8277868: Use Comparable.compare() instead of surrogate code #6575

stsypanov · 2021-11-26T12:46:59Z

Instead of something like

long x;
long y;
return (x < y) ? -1 : ((x == y) ? 0 : 1);

we can use return Long.compare(x, y);

All replacements are done with IDE.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

JDK-8277868: Use Comparable.compare() instead of surrogate code

Reviewers

Roger Riggs (@RogerRiggs - Reviewer) ⚠️ Review applies to 6744a56
Alexey Ivanov (@aivanov-jdk - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/6575/head:pull/6575
$ git checkout pull/6575

Update a local copy of the PR:
$ git checkout pull/6575
$ git pull https://git.openjdk.java.net/jdk pull/6575/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 6575

View PR using the GUI difftool:
$ git pr show -t 6575

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/6575.diff

bridgekeeper · 2021-11-26T12:47:41Z

👋 Welcome back stsypanov! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2021-11-26T12:49:58Z

@stsypanov The following labels will be automatically applied to this pull request:

client
core-libs
i18n
net

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

mlbridge · 2021-11-26T12:54:20Z

Webrevs

dfuch

Changes to java.net look good. Please obtain approval from reviewers in the other areas before integrating.

mbien · 2021-11-27T22:50:55Z

src/java.desktop/share/classes/javax/swing/plaf/basic/BasicTableUI.java

@@ -251,7 +246,7 @@ private void moveWithinTableRange(JTable table, int dx, int dy) {
        }

        private static int sign(int num) {
-            return (num < 0) ? -1 : ((num == 0) ? 0 : 1);
+            return Integer.compare(num, 0);


=> Integer.signum(num)

RogerRiggs

The core-libs file changes look fine.
A 'client' reviewer should take a look too.

openjdk · 2021-11-30T21:03:37Z

⚠️ @stsypanov the full name on your profile does not match the author name in this pull requests' HEAD commit. If this pull request gets integrated then the author name from this pull requests' HEAD commit will be used for the resulting commit. If you wish to push a new commit with a different author name, then please run the following commands in a local repository of your personal fork:

$ git checkout 8277868
$ git commit -c user.name='Preferred Full Name' --allow-empty -m 'Update full name'
$ git push

openjdk · 2021-11-30T21:03:41Z

@stsypanov 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:

8277868: Use Comparable.compare() instead of surrogate code

Reviewed-by: rriggs, aivanov

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 295 new commits pushed to the master branch:

aae5639: 8278642: Refactor java.util.Formatter
7edcd34: 8278534: Remove some unnecessary code in MethodLiveness::init_basic_blocks
0bddd8a: 8278050: Armenian text isn't rendered on macOS if text layout is performed
f6fbb5a: 8278471: Remove unreached rules in AddNode::IdealIL
0dbe4c5: 8278131: runtime/cds/appcds/dynamicArchive/* tests failing in loom repo
e6b28e0: Merge
d3408a4: 8277964: ClassCastException with no stack trace is thrown with -Xcomp in method handle invocation
69b5d49: 8272064: test/jdk/jdk/jfr/api/consumer/TestHiddenMethod.java needs update for JEP 416
d6b5544: 8278607: Misc issues in foreign API javadoc
918e339: 8278233: [macos] tools/jpackage tests timeout due to /usr/bin/osascript
... and 285 more: https://git.openjdk.java.net/jdk/compare/040b2c52d3e82048630fbd45a7db48a5e65204b7...master

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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@dfuch, @RogerRiggs, @aivanov-jdk) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

prrace · 2021-12-06T17:46:22Z

src/java.desktop/share/classes/java/awt/geom/Line2D.java

@@ -112,15 +112,15 @@ public Float(Point2D p1, Point2D p2) {
         * @since 1.2
         */
        public double getX1() {
-            return (double) x1;
+            return x1;


What do these changes have to do with the subject of the PR ?

Just a tiny clean-up in on of affected files. Do you want this to be reverted?

I believe it makes sense to preserve the cast: the fields are of type float and explicit cast hints there's a type conversion.

prrace · 2021-12-06T17:48:37Z

src/java.desktop/share/classes/sun/java2d/Spans.java


-            return result;
+            return Float.compare(mStart, otherStart);


We don't need the variable any more, do we ?

aivanov-jdk · 2021-12-07T12:01:27Z

src/java.base/share/classes/java/util/Calendar.java

@@ -3417,7 +3417,7 @@ private void updateTime() {

    private int compareTo(long t) {
        long thisTime = getMillisOf(this);
-        return (thisTime > t) ? 1 : (thisTime == t) ? 0 : -1;
+        return Long.compare(thisTime, t);


Probably, in this case thisTime variable can also be dropped.

aivanov-jdk · 2021-12-07T12:09:42Z

src/java.base/share/classes/java/util/Date.java

    public int compareTo(Date anotherDate) {
        long thisTime = getMillisOf(this);
        long anotherTime = getMillisOf(anotherDate);
-        return (thisTime<anotherTime ? -1 : (thisTime==anotherTime ? 0 : 1));
+        return Long.compare(thisTime, anotherTime);


Looks like local variables can also be dropped here as each value is used once.

aivanov-jdk · 2021-12-07T12:15:04Z

src/java.base/share/classes/java/util/UUID.java

-                 (this.leastSigBits < val.leastSigBits ? -1 :
-                  (this.leastSigBits > val.leastSigBits ? 1 :
-                   0))));
+                 Long.compare(this.leastSigBits, val.leastSigBits)));


In this case Javadoc specifies only -1, 0 or 1 can be returned. Long.compare does not specify this but returns these values. Couldn't it cause any problems in the future if implementation of Long.compare is changed?

Does it make sense to use Long.compare for mostSigBits too?

int mostSigBits = Long.compare(this.mostSigBits, val.mostSigBits); return mostSigBits != 0 ? mostSigBits : Long.compare(this.leastSigBits, val.leastSigBits);

This behavior is specified in Comparator itself, so is hardly to be ever changed, I think

This behavior is specified in Comparator itself, so is hardly to be ever changed, I think

Not quite. Comparator.compare returns: “a negative integer, zero, or a positive integer as the first argument is less than, equal to, or greater than the second.” Comparable.compareTo does the same. Yet Javadoc for UUID specifically requires -1, 0 or 1, which is stricter. However, the implementation of Long.compare complies with this requirement even though it's not specified.

Probably, it makes sense to update the Javadoc for UUID and relax the requirement to “a negative integer, zero, or a positive integer.”

I guess I didn't make myself clear. It wasn't a showstopper but it's still an issue.

This kind of change to Javadoc requires a CSR therefore it's better to separate it from this changeset which touches other files.

I see two options: 1) leave this unchanged and update implementation together with Javadoc; or 2) integrate the update to the implementation which you already have and update the Javadoc in a follow-up bug.

The suggestion to compare mostSigBits using Long.compare if it looks reasonable still applies.

Ok, let me revert JavaDoc and apply the suggestion

aivanov-jdk · 2021-12-07T12:20:19Z

src/java.desktop/share/classes/java/awt/geom/Line2D.java

@@ -112,15 +112,15 @@ public Float(Point2D p1, Point2D p2) {
         * @since 1.2
         */
        public double getX1() {
-            return (double) x1;
+            return x1;


I believe it makes sense to preserve the cast: the fields are of type float and explicit cast hints there's a type conversion.

This reverts commit f3b95d0.

stsypanov · 2021-12-16T11:52:26Z

/integrate

openjdk · 2021-12-16T11:53:11Z

@stsypanov
Your change (at version d15e03a) is now ready to be sponsored by a Committer.

RogerRiggs · 2021-12-16T21:37:23Z

/sponsor

openjdk · 2021-12-16T21:39:19Z

Going to push as commit 20db780.
Since your change was applied there have been 299 commits pushed to the master branch:

937126b: 8278551: Shenandoah: Adopt WorkerThread::worker_id() to replace Shenandoah specific implementation
f15a59c: 8278756: Parallel: Drop PSOldGen::_reserved
271d26f: 8278601: Parallel: Remove redundant code in ObjectStartArray::initialize
e0700bf: 8278763: Parallel: Remove grows_up/grows_down in PSVirtualSpace
aae5639: 8278642: Refactor java.util.Formatter
7edcd34: 8278534: Remove some unnecessary code in MethodLiveness::init_basic_blocks
0bddd8a: 8278050: Armenian text isn't rendered on macOS if text layout is performed
f6fbb5a: 8278471: Remove unreached rules in AddNode::IdealIL
0dbe4c5: 8278131: runtime/cds/appcds/dynamicArchive/* tests failing in loom repo
e6b28e0: Merge
... and 289 more: https://git.openjdk.java.net/jdk/compare/040b2c52d3e82048630fbd45a7db48a5e65204b7...master

Your commit was automatically rebased without conflicts.

openjdk · 2021-12-16T21:39:40Z

@RogerRiggs @stsypanov Pushed as commit 20db780.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

RogerRiggs · 2021-12-17T15:03:14Z

As it turns out replacing the code was not 100% equivalent and a test failure resulted.
See https://bugs.openjdk.java.net/browse/JDK-8278937

Double.compare and the original code handle the non-numeric forms of Double differently.

8277868: Use Comparable.compare() instead of surrogate code

Verified

This commit was signed with the committer’s verified signature.

UdjinM6

GPG key ID: 83592BD1400D58D9

Verified
Learn about vigilant mode

8fa8242

openjdk bot added the rfr label Nov 26, 2021

openjdk bot added client core-libs net i18n labels Nov 26, 2021

dfuch reviewed Nov 26, 2021

View reviewed changes

mbien reviewed Nov 27, 2021

View reviewed changes

8277868: Use Integer.signum() in BasicTableUI

6744a56

RogerRiggs approved these changes Nov 30, 2021

View reviewed changes

openjdk bot added the ready label Nov 30, 2021

prrace reviewed Dec 6, 2021

View reviewed changes

8277868: Inline local var

1b3a5d4

aivanov-jdk reviewed Dec 7, 2021

View reviewed changes

stsypanov added 5 commits December 7, 2021 16:32

8277868: Inline local vars

79f36a8

8277868: Revert irrelevant changes

bb06bf2

8277868: Adjust JavaDoc of UUID

f3b95d0

Revert "8277868: Adjust JavaDoc of UUID"

aa6604f

This reverts commit f3b95d0.

8277868: Use Long.compare() for mostSigBits as well

d15e03a

aivanov-jdk approved these changes Dec 9, 2021

View reviewed changes

openjdk bot added the sponsor label Dec 16, 2021

openjdk bot added the integrated label Dec 16, 2021

openjdk bot closed this Dec 16, 2021

openjdk bot removed ready rfr sponsor labels Dec 16, 2021

stsypanov deleted the 8277868 branch December 17, 2021 09:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8277868: Use Comparable.compare() instead of surrogate code #6575

8277868: Use Comparable.compare() instead of surrogate code #6575

stsypanov commented Nov 26, 2021 •

edited by openjdk bot

Loading

bridgekeeper bot commented Nov 26, 2021

openjdk bot commented Nov 26, 2021

mlbridge bot commented Nov 26, 2021 •

edited

Loading

dfuch left a comment

mbien Nov 27, 2021

stsypanov Nov 29, 2021

RogerRiggs left a comment

openjdk bot commented Nov 30, 2021

openjdk bot commented Nov 30, 2021 •

edited

Loading

prrace Dec 6, 2021

stsypanov Dec 7, 2021

aivanov-jdk Dec 7, 2021 •

edited

Loading

stsypanov Dec 8, 2021

prrace Dec 6, 2021

stsypanov Dec 7, 2021

aivanov-jdk Dec 7, 2021

stsypanov Dec 7, 2021

aivanov-jdk Dec 7, 2021

stsypanov Dec 7, 2021

aivanov-jdk Dec 7, 2021

stsypanov Dec 8, 2021

aivanov-jdk Dec 8, 2021

stsypanov Dec 9, 2021

aivanov-jdk Dec 9, 2021

aivanov-jdk Dec 9, 2021

stsypanov Dec 9, 2021

aivanov-jdk Dec 7, 2021 •

edited

Loading

stsypanov commented Dec 16, 2021

openjdk bot commented Dec 16, 2021

RogerRiggs commented Dec 16, 2021

openjdk bot commented Dec 16, 2021

openjdk bot commented Dec 16, 2021

RogerRiggs commented Dec 17, 2021

8277868: Use Comparable.compare() instead of surrogate code #6575

8277868: Use Comparable.compare() instead of surrogate code #6575

Conversation

stsypanov commented Nov 26, 2021 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Nov 26, 2021

openjdk bot commented Nov 26, 2021

mlbridge bot commented Nov 26, 2021 • edited Loading

Webrevs

dfuch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RogerRiggs left a comment

Choose a reason for hiding this comment

openjdk bot commented Nov 30, 2021

openjdk bot commented Nov 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aivanov-jdk Dec 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aivanov-jdk Dec 7, 2021 • edited Loading

Choose a reason for hiding this comment

stsypanov commented Dec 16, 2021

openjdk bot commented Dec 16, 2021

RogerRiggs commented Dec 16, 2021

openjdk bot commented Dec 16, 2021

openjdk bot commented Dec 16, 2021

RogerRiggs commented Dec 17, 2021

stsypanov commented Nov 26, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented Nov 26, 2021 •

edited

Loading

openjdk bot commented Nov 30, 2021 •

edited

Loading

aivanov-jdk Dec 7, 2021 •

edited

Loading

aivanov-jdk Dec 7, 2021 •

edited

Loading