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

[Phil Race]

On 05/31/2016 12:20 PM, Jim Graham wrote:
> 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.

Oh yeah ! CCM is gone from the latest webrev. I expect that was not 
intentional.

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

Neither ColorSpace nor ICC_ColorSpace over-ride equals or hashCode and
all the predefined ones are constructed as singletons so it seems unlikely
to cause problems

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