RFR: 8263436: Silly array comparison in GaloisCounterMode.overlapDetection

[Aleksey Shipilev]

On Thu, 11 Mar 2021 17:36:00 GMT, Anthony Scarpino <ascarpino at openjdk.org> wrote:

>> SonarCloud reports:
>>   Use "Arrays.equals(array1, array2)" or the "==" operator instead of using the "Object.equals(Object obj)" method.
>> 
>>         } else if (!src.isDirect() && !dst.isDirect()) {
>>             if (!src.isReadOnly()) {
>>                 // If using the heap, check underlying byte[] address.
>>                 if (!src.array().equals(dst.array()) ) { // <--- here
>> 
>> Additional testing:
>>   - [x] Linux x86_64 fastdebug `jdk_security`
>
> Can you explain why the silly way it is written now is any different than what you are proposing? When running jshell your proposed change returns the same result as the existing code:
> 
> one ==> byte[5] { 1, 2, 3, 4, 5 }
> two ==> byte[5] { 1, 2, 3, 4, 5 }
> jshell> one != two
> $9 ==> true
> 
> jshell> !one.equals(two)
> $10 ==> true
> 
> jshell> one != one
> $11 ==> false
> 
> jshell> !one.equals(one)
> $12 ==> false
> 
> Is the analysis tool thinking equals() is comparing the contents?

> Can you explain why the silly way it is written now is any different than what you are proposing? Is the analysis tool thinking equals() is comparing the contents?

See: https://rules.sonarsource.com/java/RSPEC-2159.

Using `equals` raises the question if actual array contents comparisons was implied (which would require `Arrays.equals`). I know it is not implied here. Using `==` makes the intent clear. This is why the rule asks to choose either `==` or `Arrays.equals`. This is the only instance where that rule triggers in whole OpenJDK code.

>From a micro-optimization perspective, `equals` is a method. This means interpreters are would have to do that call. Compilers would hopefully inline the call, but then it would still imply the null-check, because NPE should be thrown if one side is `null`.

So, why call `equals()`, when `==` is better?

-------------

PR: https://git.openjdk.java.net/jdk/pull/2938