[OpenJDK 2D-Dev] Review Request for JDK-7107905: equals() method in IndexColorModel doesnt exist and it relies on ColorModel.equals() which is not strict

[Jim Graham]

Are there any bugs in the database about the fact that many of these 
ColorModel subclasses implement equals without hashcode?  They really 
should both be implemented, but since ColorModel implements the method 
based on its tests, they are technically covered by the equals/hashCode 
contract - it's just not a very optimal implementation of the contract.

For now, it would be good to implement hashCode() on ICM now that you 
are creating an equals() method for it.

Also, ColorModel.hashCode() is a poor implementation.  It doesn't use 
the paradigms recommended by Effective Java and looks like it produces 
poor hashes as a result (just in the first two elements of the hashCode 
I see a collision due to poor choice of numbers).

With respect to this particular equals() override I think it looks fine, 
though the use of the isValid() method reduces its performance for a 
couple of reasons:
- it retests the index for the range [0,map size] which we already know 
is valid
- if the validBits is null, there is no need to even do the test.
This might be faster for the very common case:

     if (validBits == null) {
         if (cm.validBits != null) return false;
         // loop just comparing rgb[] values
     } else {
         if (cm.validBits == null) return false;
         // loop, testing rgb[] values and
         // corresponding validBits indices directly
     }

Note that in the constructor we set validBits to null if it is "true" 
for all valid indices so if one of the cmaps has it null and the other 
does not, then that is a very good indicator that their valid indices 
don't match.

On a more minor note, I don't like the indentation of the if statement, 
I'd move the "{" brace to a line of its own indented the same as its 
closing "}" to make the body of the if stand out.  It isn't strictly the 
Java coding guidelines, but it does match a bunch of the other 2D code - 
sort of a local exception to the coding style.

I'd also add a few test cases that test that 2 separately and 
identically constructed ICM instances are equals() == true, tested with 
one of each of the constructors...

			...jim

On 4/6/2016 4:47 AM, Jayathirth D V wrote:
> Hi,
>
> _Please review the following fix in JDK9:_
>
> Bug : https://bugs.openjdk.java.net/browse/JDK-7107905
>
> Webrev : http://cr.openjdk.java.net/~jdv/7107905/webrev.00/
>
> Issue : When we compare two IndexColorModel objects with different
> ColorMap(or any other property specific to IndexColorModel) using
> equals() method it returns true.
>
> Root cause : There is no override equals() method specific to
> IndexColorModel and it uses super class equals() which is ColorModel and
> it doesn’t verify equality for IndexColorModel specific properties.
>
> Solution : Add override equals() method for IndexColorModel which
> verifies its unique properties for equality.
>
> Thanks,
>
> Jay
>