[OpenJDK 2D-Dev] Review Request for JDK-7107905: ColorModel subclasses are missing hashCode() or equals() or both methods

[Jim Graham]

Hi Jay,

You were going to remove hashCode/equals from CCM, but instead you 
simply removed it from the patch.  You still need to edit it to remove 
the existing methods.

Also, I'm not sure == is a good way to compare ColorSpace instances - Phil?

			...jim

On 05/29/2016 11:55 PM, Jayathirth D V wrote:
> Hi Jim,
>
> Thanks for your valuable inputs. I have made recommended changes.
> Please find updated webrev for review:
> http://cr.openjdk.java.net/~jdv/7107905/webrev.08/
>
> Thanks,
> Jay
>
> -----Original Message-----
> From: Jim Graham
> Sent: Friday, May 27, 2016 11:52 PM
> To: Jayathirth D V; Philip Race
> Cc: 2d-dev at openjdk.java.net
> Subject: Re: Review Request for JDK-7107905: ColorModel subclasses are missing hashCode() or equals() or both methods
>
> I think that makes sense.  If it has no type-specific information to compare, then it doesn't need to override them...
>
> 			...jim
>
> On 05/27/2016 05:32 AM, Jayathirth D V wrote:
>> Hi Jim,
>>
>> Since we will be moving comparison for ColorSpace and transferType to CM, can we remove equals() & hashCode() methods in CCM?
>> For PCM and ICM we have unique variables which we can compare, there will be no problem.
>>
>> Thanks,
>> Jay
>>
>> -----Original Message-----
>> From: Jim Graham
>> Sent: Friday, May 27, 2016 2:18 PM
>> To: Jayathirth D V; Philip Race
>> Cc: 2d-dev at openjdk.java.net
>> Subject: Re: Review Request for JDK-7107905: ColorModel subclasses are
>> missing hashCode() or equals() or both methods
>>
>> Hi Jay,
>>
>> .equals() should not be comparing any fields that are computed from other fields - generally fields that come directly from a constructor argument tend to be the fields to compare.  Internal state variables (such as "isInited" and the like) should never be compared.  It should be comparing fields that affect the interpretation of the color.  To that end...
>>
>> In CM, add comparison/hash of:
>>       ColorSpace
>>       transferType
>>
>> In CCM, do not compare or hash:
>>       needScaleInit (internal state for lazy ScaleInit)
>>       noUnnorm (computed from other constructor arguments)
>>       nonStdScale (computed from other constructor arguments)
>>       signed (computed from other constructor arguments)
>>       transferType (tested in CM)
>> (basically, CCM doesn't have anything to compares as all of its
>> significant values are compared in the super class)
>>
>> 			...jim
>>
>> On 05/27/2016 01:02 AM, Jayathirth D V wrote:
>>> Hi,
>>>
>>> Gentle reminder.
>>> Please review updated fix:
>>> http://cr.openjdk.java.net/~jdv/7107905/webrev.07/
>>>
>>> Thanks,
>>> Jay
>>>
>>> -----Original Message-----
>>> From: Jayathirth D V
>>> Sent: Thursday, May 19, 2016 6:43 PM
>>> To: Philip Race; Jim Graham
>>> Cc: 2d-dev at openjdk.java.net
>>> Subject: Review Request for JDK-7107905: ColorModel subclasses are
>>> missing hashCode() or equals() or both methods
>>>
>>> Hi,
>>>
>>> Previously for this bug we were making changes related only to IndexColorModel. Since we are expanding to include hashCode() or equals() method from PackedColorModel and ComponentColorModel, I have created single webrev for review under the same bug id.
>>>
>>> Now the "getclass()==" check is present in base class ColorModel and each subclass of ColorModel have their own equality check for properties.
>>>
>>> Details:
>>> Bug : https://bugs.openjdk.java.net/browse/JDK-7107905
>>> Updated Webrev : http://cr.openjdk.java.net/~jdv/7107905/webrev.07/
>>>
>>> Please review the changes at your convenience.
>>>
>>> Thanks,
>>> Jay
>>>