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

[Jayathirth D V]

Hi Jim,

Thanks for your valuable inputs. 
Please find the updates inline. I have created new webrev http://cr.openjdk.java.net/~jdv/7107905/webrev.01/  for review.

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.

	- Yes https://bugs.openjdk.java.net/browse/JDK-6588409 has mentioned the same thing. Can I create a subtask to address java.awt.image changes?

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

	- I am not completely sure of what is the optimal way of adding hashCode() API so I took help from internet and IDE to make the changes. I am including super.hashCode() as it adds uniqueness to ICM.

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).
	- I think since we are not using Prime numbers and it might result in improper hash code. I have made similar changes to hashCode() of ColorModel.

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.

	- I have updated the suggested changes.

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.

	- In the same file itself we are following Java coding guidelines of having braces after if like "if () {". I am not completely sure of including "{" in new line.

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...

	- I have made changes to test case for verifying all constructors with same ICM. Also added verification for hashCode value.

			...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
>