[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:16 PM, Jim Graham wrote:
> 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"...?

That wording works for me. I just thought someone else might prefer 
'type' strongly enough to ask for it.

-phil.

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