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