On Jun 12, 2013, at 7:18 AM, Paul Sandoz <paul.san...@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
