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

Paul Sandoz paul.sandoz at oracle.com
Wed Jun 12 14🔞09 UTC 2013

Previous message: RFR 8009736: Comparator API cleanup
Next message: RFR 8009736: Comparator API cleanup
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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

Previous message: RFR 8009736: Comparator API cleanup
Next message: RFR 8009736: Comparator API cleanup
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

More information about the core-libs-dev mailing list