RFR 8009736: Comparator API cleanup (original) (raw)

Henry Jen henry.jen at Oracle.com
Wed Jun 12 15:54:28 UTC 2013

On Jun 12, 2013, at 7:18 AM, Paul Sandoz <paul.sandoz at oracle.com> wrote:

Hi Henry,

If you don't mind could you hold off on committing this one until the following have gone through to tl: http://cr.openjdk.java.net/~psandoz/tl/JDK-8016251-spinedBuffer-splitr/webrev/ http://cr.openjdk.java.net/~psandoz/tl/JDK-8016308-Node/webrev/ http://cr.openjdk.java.net/~psandoz/tl/JDK-8016324-pipelines/webrev/ http://cr.openjdk.java.net/~psandoz/tl/JDK-8016455-stream-tests/webrev/ as there is a delicate balance here to group the code into meaningful units for review and it's awkward to shuffle/update things. I rebased your patch off my patch queue, only one conflict required fixing. I can send you that queue in another email. Is that OK for you?

Sure.

--

Comparator.reverseOrder + *

The returned comparator is serializable. Try to compare null with

+ * returned comparator will throw {@link NullPointerException}. + * Typo "Try to compare" (and to . Do you mean: The compare method of the returned comparator will throw a {@link NullPointerException} if a {@code null} value is passed as the second parameter. ? Perhaps add a "@See Collections#reverseOrder" and vice versa on that method. Similar issue for Comparator.naturalOrder but for null passed as the first parameter.

Is "compare null using" better then "compare null with"?

null passed as an argument will cause NPE on returned comparator, regardless position.

Comparators.comparing. Rather than "For example" you can use @apiNote .e.g * @apiNote * To obtain a .... The ToIntFunction accepting method has an example where as the long and double methods to not, perhaps just remove it.

Make sense. I have #see for comparing(Function), should be clear enough.

Comparators.NullComparator The "real" field can never be null: + NullComparator(int sign, Comparator<? super T> real) { + this.sign = sign; + this.real = (Comparator) Objects.requireNonNull(real); + } but you are checking for null in the compare method + @Override + public int compare(T a, T b) { + if (a == null) { + return (b == null) ? 0 : sign; + } else if (b == null) { + return -sign; + } else { + return (real == null) ? 0 : real.compare(a, b); // <---- null check + } + }

Hmm, I missed this one. Thanks.

Map.comparingByKey/Value(Comparator<? super K/V> cmp) You don't mention "Note that a null key/value…"

That's because null is handled by Comparator in this case, if the Comparator is null-friendly, it is fine. Perhaps I should make that clear.

test/java/util/Comparator/BasicTest.java test/java/util/Comparator/TypeTest.java Has the wrong license header: + * This code is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License version 2 only, as + * published by the Free Software Foundation. Oracle designates this + * particular file as subject to the "Classpath" exception as provided + * by Oracle in the LICENSE file that accompanied this code. should be: + * This code is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License version 2 only, as + * published by the Free Software Foundation.

WIll fix.

Thanks for review.

Cheers, Henry

More information about the core-libs-dev mailing list