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

[Phil Race]

+1

-phil.

On 06/02/2016 07:07 AM, Jayathirth D V wrote:
> Hi Phil,
>
> I have updated the code with all the changes I mentioned previously. I am caching hashCode when first time hashCode() is called.
> Please find the updated webrev for review:
> http://cr.openjdk.java.net/~jdv/7107905/webrev.09/
>
> Thanks,
> Jay
>
> -----Original Message-----
> From: Philip Race
> Sent: Wednesday, June 01, 2016 10:06 PM
> To: Jayathirth D V
> Cc: Jim Graham; 2d-dev at openjdk.java.net
> Subject: Re: Review Request for JDK-7107905: ColorModel subclasses are missing hashCode() or equals() or both methods
>
> Please post the updated webrev.
>
> -phil.
>
> On 6/1/16, 12:02 AM, Jayathirth D V wrote:
>> Hi Phil&  Jim,
>>
>> Collating all the changes needed:
>> 1) I have removed both equals()&  hashCode() in CCM but forgot to add the file while creating webrev. I will include changes in CCM in updated webrev.
>> 2) Caching of hashCode value in IndexColorModel&  PackedColorModel :
>> 	We can cache hashCode value when constructor is called or when hashCode() is called for first time. What approach we have to follow?
>> 3) Comment section of equals() method, I will update it to :
>> 	1449      * the target object must be the same class (and not a subclass) as this
>> 4) I will use .equals() to compare colorSpace values in CM.equals() so it will be like we are not intending ColorSpace class to never override equals() method.
>>
>> Please let me know your inputs.
>>
>> Thanks,
>> Jay
>>
>> -----Original Message-----
>> From: Jim Graham
>> Sent: Wednesday, June 01, 2016 4:14 AM
>> To: Phil Race
>> Cc: Jayathirth D V; 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 we should use .equals() here, otherwise it looks like a recommendation that the intent is for those classes to never implement it...
>>
>> 			...jim
>>
>> On 05/31/2016 03:31 PM, Phil Race wrote:
>>> I don't know of any design intent, so yes, I meant more as a
>>> practical matter.
>>> Unless they subclass then using equals() which I thought unlikely it
>>> makes no difference here.
>>> But it would be proof against that to use equals, unlikely to matter
>>> as it might be ..
>>>
>>> -phil.
>>>
>>> On 05/31/2016 03:19 PM, Jim Graham wrote:
>>>> On 05/31/2016 02:50 PM, Phil Race wrote:
>>>>> 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
>>>> Should it use .equals() on principle, though?  Otherwise we are
>>>> baking in the assumption that it doesn't implement equals() into our
>>>> implementation of the CM.equals() method.  Might it some day
>>>> implement equals (perhaps it isn't a practical issue today, but it
>>>> might be in the future when we forget that it was omitted in this
>>>> usage we create today).
>>>>
>>>> I guess what I'm asking is if it is a design feature that different
>>>> objects are considered non-equal (such as an object that, for
>>>> instance, has only predetermined instances that are shared by
>>>> reference and reused).  I think color spaces are sort of in-between
>>>> in that we define a few constants that simply get used by reference
>>>> in 99% of cases, but that isn't a design feature, more like a
>>>> practical issue...
>>>>
>>>>               ...jim