RFR 8009736: Comparator API cleanup

[Paul Sandoz]

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?

--

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.


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.


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
+            }
+        }


Map.comparingByKey/Value(Comparator<? super K/V> cmp)

You don't mention "Note that a null key/value..."


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.

Thanks,
Paul.

On Jun 11, 2013, at 11:04 PM, Henry Jen <henry.jen at oracle.com> wrote:

> Hi,
> 
> Please review http://cr.openjdk.java.net/~henryjen/ccc/8009736.2/webrev/
> 
> Highlight of changes,
> 
> - Comparators class is now only package-private implementations. The
> public static methods have been move to other arguably more appropriate
> places, mostly in Comparator.
> 
> - Comparator.reverseOrder() is renamed to Comparator.reversed()
> 
> - nullsFirst(Comparator) and nullsLast(Comparator) are introduced to
> wrap up a comparator to be null-friendly.
> 
> To see the API changes, found the specdiff at
> http://cr.openjdk.java.net/~henryjen/ccc/8009736.2/specdiff/overview-summary.html
> 
> Cheers,
> Henry