On Mon, 19 May 2025 17:06:17 GMT, Raffaello Giulietti <rgiulie...@openjdk.org> wrote:
>> 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. > > @amaembo Could you please switch the test class to make use of > [JUnit](https://junit.org/junit5/)? For new tests we now prefer JUnit over > TestNG. > Thanks! @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`](https://guava.dev/releases/snapshot-jre/api/docs/com/google/common/collect/Ordering.html#max(E,E)) (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 `<=`. ------------- PR Comment: https://git.openjdk.org/jdk/pull/25297#issuecomment-2897204193
