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

[Jim Graham]

Hi Phil,

I don't think that alternate wording conveys that this is not a case of 
"instanceof" and is instead a case of "getClass() =="...

Perhaps we don't need "exact", but "type" is vague enough to call into 
question how we do the test.  I could see "the same class (and not a 
subclass) as this"...?

			...jim

On 05/31/2016 11:56 AM, Phil Race wrote:
>
> 1449      * the target object must be the exact same class as this
>
> I believe the language people like to say "the same type" so maybe say
>
> 1449      * the target object must be of the same type (i.e. class) as this
>
> Could IndexColorModel cache the hash code since it could be fairly
> expensive to compute ?
> Arrays.hashCode() is the expensive bit.
>
> Less so PackedColorModel but could be considered there too.
>
> No other comments at this time.
>
> -phil.
>
> 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
>>>>
>