8356995: Provide default methods min(T, T) and max(T, T) in Comparator interface #25297

amaembo · 2025-05-19T07:25:17Z

Implementation of Comparator.min and Comparator.max methods. Preliminary discussion is in this thread:
https://mail.openjdk.org/pipermail/core-libs-dev/2025-May/145638.html
The specification is mostly composed of Math.min/max and Collections.min/max specifications.

The methods are quite trivial, so I don't think we need more extensive testing (e.g., using different comparators). But if you have ideas of new useful tests, I'll gladly add them.

I'm not sure whether we should specify exactly the behavior in case if the comparator returns 0. I feel that it could be a useful invariant that Comparator.min(a, b) and Comparator.max(a, b) always return different argument, partitioning the set of {a, b} objects (even if they are equal). But I'm open to suggestions here.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed (2 reviews required, with at least 2 Reviewers)
Change requires a CSR request matching fixVersion 25 to be approved (needs to be created)

Issue

JDK-8356995: Provide default methods min(T, T) and max(T, T) in Comparator interface (Enhancement - P4)(⚠️ The fixVersion in this issue is [26] but the fixVersion in .jcheck/conf is 25, a new backport will be created when this pr is integrated.)

Reviewers

Roger Riggs (@RogerRiggs - Reviewer) 🔄 Re-review required (review applies to 293b2c40)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/25297/head:pull/25297
$ git checkout pull/25297

Update a local copy of the PR:
$ git checkout pull/25297
$ git pull https://git.openjdk.org/jdk.git pull/25297/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 25297

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/25297.diff

Using Webrev

Link to Webrev Comment

…ator interface

amaembo · 2025-05-19T07:25:42Z

/csr

bridgekeeper · 2025-05-19T07:26:10Z

👋 Welcome back tvaleev! 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 · 2025-05-19T07:26:16Z

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

openjdk · 2025-05-19T07:26:46Z

@amaembo has indicated that a compatibility and specification (CSR) request is needed for this pull request.

@amaembo please create a CSR request for issue JDK-8356995 with the correct fix version. This pull request cannot be integrated until the CSR request is approved.

openjdk · 2025-05-19T07:27:15Z

@amaembo The following label will be automatically applied to this pull request:

core-libs

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

mlbridge · 2025-05-19T07:34:42Z

Webrevs

archiecobbs · 2025-05-19T14:59:05Z

I'm not sure whether we should specify exactly the behavior in case if the comparator returns 0. I feel that it could be a useful invariant that Comparator.min(a, b) and Comparator.max(a, b) always return different argument, partitioning the set of {a, b} objects (even if they are equal). But I'm open to suggestions here.

IMHO it makes sense. It's the min/max analog to a stable sort.

rgiulietti · 2025-05-19T17:06:17Z

@amaembo Could you please switch the test class to make use of JUnit? For new tests we now prefer JUnit over TestNG.
Thanks!

RogerRiggs

Min and Max for the equals case would be more consistent if the value returned for equals was the same as the condition, as if => or <= were used for the comparison.
The code would be easier to read and match the description if max used >= and min used <=.

…rator); junit tests

amaembo · 2025-05-21T09:11:11Z

@rgiulietti I tried to mimic the nearby tests which use testng. Converted to junit 5.

@archiecobbs

IMHO it makes sense. It's the min/max analog to a stable sort

On the second thought, there is a consistency argument. We already have BinaryOperator.minBy and BinaryOperator.maxBy, which always return the first argument in case of tie (though this is not specified, probably it should be?). So it looks like it will be better to have both APIs consistent. One more point is that Guava's Ordering (which extends Comparator) also returns the first argument in case of tie, so if we do the same, Ordering will not violate the Comparator contract. Thus I've changed the implementation to be in sync with both BinaryOperator and Ordering. This also addresses the @RogerRiggs comment: now implementations use >= and <=.

rgiulietti

A couple of nits.

rgiulietti · 2025-05-21T11:55:07Z

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

+     * @param   b   another argument.
+     * @return  the larger of {@code a} and {@code b} according to this comparator.
+     * @throws  ClassCastException if the collection contains elements that are
+     *          not <i>mutually comparable</i> (for example, strings and


 is more intent revealing.

Suggested change

* not mutually comparable (for example, strings and

* not mutually comparable (for example, strings and

I copied this sentence from java.util.Collections where  is used 48 times. I'll replace it here with , but probably it should be updated en-masse for consistency?

rgiulietti · 2025-05-21T11:55:16Z

test/jdk/java/util/Comparator/MinMaxTest.java

+
+public class MinMaxTest {
+  @Test
+  public void testMin() {


According to this, test methods do not need to be public (but cannot be private).

Suggested change

public void testMin() {

void testMin() {

Done, thanks

plevart · 2025-05-21T17:23:45Z

Hi,

There exists a class implementing Comparator and min and max methods too. But they are generic:

https://github.com/google/guava/blob/f1b962f1f1d1c96068c62b7ff55426ea249cbd55/guava/src/com/google/common/collect/Ordering.java#L622C10-L622C79

  public <E extends T> E max(E a, E b) {
    return (compare(a, b) >= 0) ? a : b;
  }

So when I have:

Comparator<CharSequence> cmp = ...

var greatest = cmp.max("foo", "bar");

...then the type of greatest would be String and not CharSequence.

RogerRiggs

Looks good. Thanks

plevart · 2025-05-21T19:20:38Z

What I was trying to say is that not only would Guava's Ordering not compile any more (it would still be binary compatible I think, as both methods have same erasure), but I also think that generic max and min as defined by Guava's Ordering are more useful too. So what do you think of making them generic like this:

    default <E extends T> E max(E e1, E e2) {
        return compare(e1, e2) >= 0 ? e1 : e2;
    }
    
    default <E extends T> E min(E e1, E e2) {
        return compare(e1, e2) <= 0 ? e1 : e2;
    }

RogerRiggs · 2025-05-21T20:59:20Z

/reviewers 2 reviewer

openjdk · 2025-05-21T20:59:43Z

@RogerRiggs
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 2 Reviewers).

amaembo · 2025-05-22T07:19:56Z

@plevart this is definitely a good idea, and it will make the methods more convenient. I used U as a new type parameter, because it looks like it's more common in the OpenJDK codebase (e.g., StructuredTaskScope::fork). Now, Guava should be source-compatible, though it's possible that some other libraries declare non-generic min/max and they will not be source-compatible anymore. I'll mention this in CSR.

RogerRiggs · 2025-05-23T18:57:45Z

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

+     *
+     * @since 25
+     */
+    default <U extends T> U max(U a, U b) {


The parameter names are o1 and o2 in the compare method min and max build on.
Though a and b are used in the class javadoc example and x and y are used in the spec description.
Can we be consistent in the API? (o1, o2) perhaps. Someday, the parameter names may be more significant.

Done, thanks.

turbanoff · 2025-05-23T20:37:05Z

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

+     * @param   b   another argument.
+     * @param   <U> the type of the arguments and the result.
+     * @return  the larger of {@code a} and {@code b} according to this comparator.
+     * @throws  ClassCastException if the collection contains elements that are


What 'collection' meaned here?

Good catch, thank you. The description was copied from Collections::max, which is not so appropriate here. I think it's better to copy it from Comparator::compare (also, added @throws NPE for consistency).

RogerRiggs

Re-reviewed the PR and CSR. Thanks for the updates. Looks good.
Set the fixVersion to 25 and Finalize or at least Proposed to start the CSR review.

amaembo · 2025-05-27T18:23:24Z

@RogerRiggs thank you! I always forget that CSR should be moved forward by its author...

…into 25

JDK-8356995 Provide default methods min(T, T) and max(T, T) in Compar…

d72c0e8

…ator interface

openjdk bot added the csr Pull request needs approved CSR before integration label May 19, 2025

openjdk bot added the core-libs [email protected] label May 19, 2025

Fix linebreaks

2cda59b

openjdk bot added the rfr Pull request is ready for review label May 19, 2025

RogerRiggs reviewed May 19, 2025

View reviewed changes

Return first argument in case of tie (to be consistent with BinaryOpe…

9edc73e

…rator); junit tests

rgiulietti reviewed May 21, 2025

View reviewed changes

RogerRiggs approved these changes May 21, 2025

View reviewed changes

amaembo added 3 commits May 22, 2025 09:02

Use instead of 

f6fc4a9

MinMaxTest: remove unnecessary public modifier

b750c3f

Make min and max generic

38f9eb7

RogerRiggs reviewed May 23, 2025

View reviewed changes

turbanoff reviewed May 23, 2025

View reviewed changes

Update exception description; update argument names

293b2c4

RogerRiggs approved these changes May 27, 2025

View reviewed changes

Update @SInCE tag to version 26, as it's too late to fit this change …

a278865

…into 25

	* not <i>mutually comparable</i> (for example, strings and
	* not <em>mutually comparable</em> (for example, strings and

8356995: Provide default methods min(T, T) and max(T, T) in Comparator interface #25297

Are you sure you want to change the base?

8356995: Provide default methods min(T, T) and max(T, T) in Comparator interface #25297

Conversation

amaembo commented May 19, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

amaembo commented May 19, 2025

Uh oh!

bridgekeeper bot commented May 19, 2025

Uh oh!

openjdk bot commented May 19, 2025

Uh oh!

openjdk bot commented May 19, 2025

Uh oh!

openjdk bot commented May 19, 2025

Uh oh!

mlbridge bot commented May 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

archiecobbs commented May 19, 2025

Uh oh!

rgiulietti commented May 19, 2025

Uh oh!

RogerRiggs left a comment

Choose a reason for hiding this comment

Uh oh!

amaembo commented May 21, 2025

Uh oh!

rgiulietti left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

plevart commented May 21, 2025

Uh oh!

RogerRiggs left a comment

Choose a reason for hiding this comment

Uh oh!

plevart commented May 21, 2025

Uh oh!

RogerRiggs commented May 21, 2025

Uh oh!

openjdk bot commented May 21, 2025

Uh oh!

amaembo commented May 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RogerRiggs left a comment

Choose a reason for hiding this comment

Uh oh!

amaembo commented May 27, 2025

Uh oh!

Uh oh!

amaembo commented May 19, 2025 •

edited by openjdk bot

Loading

mlbridge bot commented May 19, 2025 •

edited

Loading

amaembo commented May 22, 2025 •

edited

Loading