Re: RFR 8141409 Arrays.equals accepting a Comparator

[Paul Sandoz]

Hi Roger,

I want to keep this review focused and consistent with the existing methods 
that were previously reviewed.


> On 12 Nov 2015, at 17:42, Roger Riggs  wrote:
> 
> Hi Paul,
> 
> - A minor point but in the argument names, its a bit inconsistent between 
> using 'b' and 'a2'.
> I would have use 'a', and 'b' to be more consistent.  (and yes the current 
> methods show the same inconsistency)
> 

Perhaps, but i don’t want to do that right now.


> - The @return(s) should have an 'otherwise {@code false};  otherwise maybe it 
> is allowed to return true.
> 

The current approach is consistent with the existing equals methods.


> - In the implementation, I generally prefer the form of 
> Objects.requireNonNull(o, "name") so the exception
> identifies the offending argument.
> 
> - I thought the coding style has spaces around operators (like ==).
> 

Same as in other equals methods.


> - The non-range checked version of equals should have the same behavior as:
>   equals(a, 0, a.length, a2, 0, a2.length).
> 
> The range checked version should include the sentence:
> 
> "Also, two array references are  considered equal if both are {@code null}.”
> 

Whats the sub-range of a null array reference?

See the existing sub-range equals methods. This was discussed in the original 
review and i don’t want to reopen that decision right now.


> - The implementation will need to check for nulls before the rangeChecks.
> 

The access to the length will induce an NPE. See other methods in arrays (such 
as binarySearch)

Paul.

Re: RFR 8141409 Arrays.equals accepting a Comparator

[Roger Riggs]

Hi Paul,

- A minor point but in the argument names, its a bit inconsistent 
between using 'b' and 'a2'.
I would have use 'a', and 'b' to be more consistent.  (and yes the 
current methods show the same inconsistency)


- The @return(s) should have an 'otherwise {@code false};  otherwise 
maybe it is allowed to return true.


- In the implementation, I generally prefer the form of 
Objects.requireNonNull(o, "name") so the exception

identifies the offending argument.

- I thought the coding style has spaces around operators (like ==).

- The non-range checked version of equals should have the same behavior as:
   equals(a, 0, a.length, a2, 0, a2.length).

The range checked version should include the sentence:

"Also, two array references are  considered equal if both are {@code null}."

- The implementation will need to check for nulls before the rangeChecks.

Regards, Roger





On 11/12/2015 5:36 AM, Paul Sandoz wrote:

Hi,

I have made a retreat on this just to add the two missing equals methods:

   
http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8141409-Arrays-equals-comparator/webrev/ 


I propose to revisit the overall null-handling semantics, especially for 
Comparable[] accepting methods, at a later date.

Paul.


On 4 Nov 2015, at 15:32, Paul Sandoz  wrote:

Hi,

Please review:

  
http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8141409-Arrays-equals-comparator/webrev/ 

  https://bugs.openjdk.java.net/browse/JDK-8141409 


This builds on (the already reviewed):

  https://bugs.openjdk.java.net/browse/JDK-8033148 

  http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8033148-Arrays-lexico-compare/webrev/ 


and adds yet more methods to Arrays, only two this time though, to fill out the 
missing gaps related to array equality with a comparator.

In addition i added an Objects.compare method, which simplifies the 
implementations of some object-bearing methods.

Both additions simplify the specification some object-bearing methods.

Paul.

Re: RFR 8141409 Arrays.equals accepting a Comparator

[Paul Sandoz]

Hi,

I have made a retreat on this just to add the two missing equals methods:

  
http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8141409-Arrays-equals-comparator/webrev/
 


I propose to revisit the overall null-handling semantics, especially for 
Comparable[] accepting methods, at a later date.

Paul.

> On 4 Nov 2015, at 15:32, Paul Sandoz  wrote:
> 
> Hi,
> 
> Please review:
> 
>  
> http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8141409-Arrays-equals-comparator/webrev/
>  
> 
>  https://bugs.openjdk.java.net/browse/JDK-8141409 
> 
> 
> This builds on (the already reviewed):
> 
>  https://bugs.openjdk.java.net/browse/JDK-8033148 
> 
>  
> http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8033148-Arrays-lexico-compare/webrev/
>  
> 
> 
> and adds yet more methods to Arrays, only two this time though, to fill out 
> the missing gaps related to array equality with a comparator.
> 
> In addition i added an Objects.compare method, which simplifies the 
> implementations of some object-bearing methods.
> 
> Both additions simplify the specification some object-bearing methods.
> 
> Paul.

Re: RFR 8141409 Arrays.equals accepting a Comparator

[Paul Sandoz]

> On 4 Nov 2015, at 22:41, Paul Sandoz  wrote:
> 
>> The null handling choice seems arbitrary.
> 
> I wanted Arrays.equals to be consistent with the Comparable accepting 
> Arrays.compare and Arrays.mismatch, so it was a coin toss between nulls first 
> and nulls last, hence that choice is propagated to Objects.compare, which is 
> consistent with Objects.equals.
> 

Now i recall some more details on the decision I took. Less of a coin toss than 
it first appears.

Null array refs are also supported for Arrays.compare, again so as to be 
consistent with Arrays.equals. And it’s “natural” to consider a null array ref 
to be less than a non-null array ref value, since null is essentially 0 bits 
set, and by logical progression that can apply to any ref value.

So there are two forces pushing the design in different directions:

1) null-consistency with Arrays.equals behaviour, the current approach (and 
note that Arrays.mismatch does not support null array refs, and nor do the 
sub-range methods); and

2) consistency with Arrays.sort/binarySearch/TreeMap, where null array refs 
would not be supported and null elements would only be supported when an 
explicit null-supporting Comparator is used.

Would developers expect:

  Arrays.equals(a, b) == (Arrays.compare(a, b) == 0)

or:

  Arrays.compare(a, b) // potentially throws NPE
  Arrays.equals(a, b) == (Arrays.compare(a, b, 
Comparator.nullsFirst(Comparator.naturalOrder())) == 0)
  Arrays.equals(a, b) == (Arrays.compare(a, b, 
Comparator.nullsLast(Comparator.naturalOrder())) == 0)

?

Paul.

> The other solution, as you suggest out, is to ditch support Comparable null 
> values, where a certain choice is picked. for the relevant Comparable 
> accepting Arrays methods. That would be more consistent with existing 
> operations on Comparables, such as Arrays.sort etc.  and for consistency with 
> Arrays.equals a null accepting Comparator would need to be used with the 
> relevant methods on Arrays.
> 
> Hmm… now you have me rethinking this :-) thoughts?
> 
> Paul.
> 
>> Nulls last or nulls disallowed would be equally valid. As usual I favour 
>> nulls disallowed.
>> 

Re: RFR 8141409 Arrays.equals accepting a Comparator

[Paul Sandoz]

> On 4 Nov 2015, at 17:51, Mike Duigou  wrote:
> 
>> Date: Wed, 4 Nov 2015 15:32:25 +0100
>> From: Paul Sandoz 
>> To: core-libs-dev 
>> Subject: RFR 8141409 Arrays.equals accepting a Comparator
>> Please review:
>> In addition i added an Objects.compare method, which simplifies the
>> implementations of some object-bearing methods.
> 
> Why not put the method on Comparable?
> 

I wanted it to be located with:

  public static boolean equals(Object a, Object b)

  public static  int compare(T a, T b, Comparator c)


> The null handling choice seems arbitrary.

I wanted Arrays.equals to be consistent with the Comparable accepting 
Arrays.compare and Arrays.mismatch, so it was a coin toss between nulls first 
and nulls last, hence that choice is propagated to Objects.compare, which is 
consistent with Objects.equals.

The other solution, as you suggest out, is to ditch support Comparable null 
values, where a certain choice is picked. for the relevant Comparable accepting 
Arrays methods. That would be more consistent with existing operations on 
Comparables, such as Arrays.sort etc.  and for consistency with Arrays.equals a 
null accepting Comparator would need to be used with the relevant methods on 
Arrays.

Hmm… now you have me rethinking this :-) thoughts?

Paul.

> Nulls last or nulls disallowed would be equally valid. As usual I favour 
> nulls disallowed.
> 

Re: RFR 8141409 Arrays.equals accepting a Comparator

[Mike Duigou]

Date: Wed, 4 Nov 2015 15:32:25 +0100
From: Paul Sandoz 
To: core-libs-dev 
Subject: RFR 8141409 Arrays.equals accepting a Comparator
Please review:

In addition i added an Objects.compare method, which simplifies the
implementations of some object-bearing methods.


Why not put the method on Comparable?

The null handling choice seems arbitrary. Nulls last or nulls disallowed 
would be equally valid. As usual I favour nulls disallowed.

RFR 8141409 Arrays.equals accepting a Comparator

[Paul Sandoz]

Hi,

Please review:

  
http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8141409-Arrays-equals-comparator/webrev/
 

  https://bugs.openjdk.java.net/browse/JDK-8141409 


This builds on (the already reviewed):

  https://bugs.openjdk.java.net/browse/JDK-8033148 

  
http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8033148-Arrays-lexico-compare/webrev/
 


and adds yet more methods to Arrays, only two this time though, to fill out the 
missing gaps related to array equality with a comparator.

In addition i added an Objects.compare method, which simplifies the 
implementations of some object-bearing methods.

Both additions simplify the specification some object-bearing methods.

Paul.