[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 the review.
I have updated the changes mentioned by you. ICM.hashCode() was generated using NetBeans and I added super.hashCode() to include things from ColorModel. Initializing "hash" to 7 is redundant so I removed it and initialized to super.hashCode().

Please find updated webrev for review :
http://cr.openjdk.java.net/~jdv/7107905/webrev.06/ 

I will make changes in CCC after technical review is approved.

Thanks,
Jay
-----Original Message-----
From: Jim Graham 
Sent: Wednesday, April 27, 2016 4:24 AM
To: Jayathirth D V; Philip Race
Cc: 2d-dev at openjdk.java.net
Subject: Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905: equals() method in IndexColorModel doesnt exist and it relies on ColorModel.equals() which is not strict

This version of the ICM.equals() method starts with casting the unkown obj to ICM, which may throw a cast exception.  That cast needs to be moved down after the call to super.equals() ("this == cm" can just be "this == obj").

In the ICM hashcode function, is there a reason that the hash variable isn't just initialized to super.hashCode() rather than starting with 7 and doing a multiply-add to incorporate the super hashcode?

			...jim

On 4/25/16 1:26 AM, Jayathirth D V wrote:
> Hi,
>
> Based on Joe's and Jim's suggestion I have made changes to include check for getClass() in baseclass and use super.equals() from subclasses to verify class equality.
> Please find updated webrev for review :
> http://cr.openjdk.java.net/~jdv/7107905/webrev.05/
>
> For JDK-8153943 I will make relevant changes in its subclasses and sent review in its thread.
>
> Thanks,
> Jay
>
> -----Original Message-----
> From: Jim Graham
> Sent: Thursday, April 14, 2016 2:57 AM
> To: Jayathirth D V; Philip Race
> Cc: 2d-dev at openjdk.java.net
> Subject: Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905: equals() 
> method in IndexColorModel doesnt exist and it relies on 
> ColorModel.equals() which is not strict
>
> Looks good.  The only thing left is the CCC...
>
> 			...jim
>
> On 4/13/16 1:33 AM, Jayathirth D V wrote:
>> Hi,
>>
>> Thanks Phil & Jim for your suggestion.
>> I have made changes mentioned by Jim for  whether we have validate individual bits of validBits or not in different conditions.
>> Please find updated webrev for review :
>> http://cr.openjdk.java.net/~jdv/7107905/webrev.04/
>>
>> Thanks,
>> Jay
>>
>> -----Original Message-----
>> From: Phil Race
>> Sent: Wednesday, April 13, 2016 1:49 AM
>> To: Jim Graham
>> Cc: Jayathirth D V; Prasanta Sadhukhan; 2d-dev at openjdk.java.net
>> Subject: Re: Review Request for JDK-7107905: equals() method in 
>> IndexColorModel doesnt exist and it relies on ColorModel.equals() 
>> which is not strict
>>
>> Seems like this would work/help.
>>
>> -phil.
>>
>> On 04/12/2016 01:19 PM, Jim Graham wrote:
>>>
>>>
>>> On 4/12/2016 12:59 PM, Phil Race wrote:
>>>> I am catching up on email here, and "+1" but a couple of comments
>>>>
>>>> - I suppose that we can't learn anything useful from 
>>>> "cm.validbits.equals(this.validbits)"
>>>> since only the bits up to "map_size" should be tested ?
>>>
>>> Perhaps if the constructors truncated it at the required size we 
>>> could use equals().  I'm not sure that is worthwhile, given how 
>>> rarely it is used.  I think it is used by default on some platforms (Windows?
>>> X11?) if they have an 8-bit screen with a sparse colormap, but that 
>>> should be pretty rare these days since nearly everything we use 
>>> should be 24-bits these days.
>>>
>>> I have a better idea, though.
>>>
>>> But, since the field is assigned the original supplied value from 
>>> the constructor, then the likelihood that a non-null value will be 
>>> == identical to another colormap is likely larger than normal, 
>>> perhaps we can make it faster by checking for == and then slipping 
>>> into the faster test that simply compares the rgb[] values? Something like:
>>>
>>> if (validBits == cm.validBits) {
>>>      compare rgb[] entries
>>> } else if (validBits == null || cm.validBits == null) {
>>>      return false;
>>> } else {
>>>      bigger loop that compares rgb[] and validBits() values }
>>>
>>> Note that if the two fields are == and non-null, then the entries in 
>>> the rgb[] array for indices that are non-valid should have zeros in 
>>> them because of the way that the colormap data is copied internally, 
>>> so the rgb[] comparisons should be valid without checking the 
>>> associated bits.
>>>
>>> Potentially we could also use:
>>>
>>> boolean fulltest;
>>> if (validBits == cm.validBits) {
>>>      fulltest = false;
>>> } else if (validBits == null || cm.validBits == null) {
>>>      return false;
>>> } else if (validBits.equals(cm.validBits)) {
>>>      fulltest = false;
>>> } else {
>>>      fulltest = true;
>>> }
>>>
>>> if (fulltest) {
>>>      compare both rgb[] and validBits() values } else {
>>>      compare rgb[] entries alone
>>> }
>>>
>>> In this case we are using validBits.equals() to reduce the amount of 
>>> testing in the loop, but are not basing a conclusion on whether the 
>>> result of the comparison will be true or false.  equals() implies we 
>>> don't need to compare its values bit by bit, but !equals() doesn't 
>>> mean that the ICMs aren't equals()...
>>>
>>>              ...jim
>>