RFR 8009736: Comparator API cleanup

[Remi Forax]

On 06/14/2013 12:07 PM, Paul Sandoz wrote:
> On Jun 13, 2013, at 7:28 PM, Tom Hawtin <tom.hawtin at oracle.com> wrote:
>
>> On 12/06/2013 17:36, Paul Sandoz wrote:
>>
>>> "The returned comparator is serializable and does not permit null arguments when comparing objects".
>> Being Serializable at all is odd. It brings the previously
>> implementation-dependent name and fields into the API spec.
>>
> That is, alas, inherently the nature of serialization. Collections are serializable, some collections take Comparator instances on construction (e.g. TreeSet).
>
>
>> Serialisability also allows curious behaviour. Whilst not, say, a
>> vulnerability itself, oddities aren't generally a good idea. For an
>> example of strangeness, NullComparator can have a sign of
>> -Integer.MIN_VALUE. It can also have a `real` field of null which is
>> otherwise checked for during construction,
> I dunno what to say except "serialization is bad! i wish it were never in the JDK" :-)
>
>
>> although strangely the
>> compare method also checks whether real is null or not.
>>
> Yes, Henry is gonna fix that null check.
>
> FWIW i find this marginally clearer:
>
>          NullComparator(boolean nullsFirst, Comparator<? super T> real) {
>              this(nullsFirst ? -1 : 1, real);
>          }
>
>          private NullComparator(int sign, Comparator<? super T> real) {
>              this.sign = sign;
>              this.real = (Comparator<T>) Objects.requireNonNull(real);
>          }
>
> but it's just nit-picking.

and why the NullComparator is specified as a class an not as a 
serializable lambda
(and please casting a Comparator<? super T> to a Comparator<T> is 
unsafe, real should be declared as a Comparator<? super T>).

>
> Paul.
>
>

Rémi