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

[Jayathirth D V]

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