RFR 8009736: Comparator API cleanup

[Henry Jen]

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
> 
> +     * <p>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<T>) 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