Hi, Since Joe mentioned to add information related to what fields we are using to calculate equals() method in ColorModel and its subclasses I have updated the spec for the same.
Please find updated webrev for review: http://cr.openjdk.java.net/~jdv/7107905/webrev.11/ Thanks, Jay -----Original Message----- From: Jim Graham Sent: Saturday, June 04, 2016 4:52 AM To: Jayathirth D V; Philip Race Cc: 2d-dev@openjdk.java.net Subject: Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905: ColorModel subclasses are missing hashCode() or equals() or both methods That looks good to me. Has the CCC cleared yet? ...jim On 6/3/16 2:49 AM, Jayathirth D V wrote: > Hi Jim, > > Oh that's an important change. > Thanks for your review. > > I have updated ColorModel constructor to copy nBIts only of numOfComponents > length and I have removed validBits check in hashCode() of ICM. > Please find updated webrev for review: > http://cr.openjdk.java.net/~jdv/7107905/webrev.10/ > > Thanks, > Jay > > -----Original Message----- > From: Jim Graham > Sent: Friday, June 03, 2016 2:25 AM > To: Jayathirth D V; Philip Race > Cc: 2d-dev@openjdk.java.net > Subject: Re: Review Request for JDK-7107905: ColorModel subclasses are > missing hashCode() or equals() or both methods > > I just noticed a hashCode/equals violation that we created a few revisions > ago. We compute the hash of the validBits in ICM, but we only compare > validBits up to the number of colors in the colormap. One could construct > two ICMs that have different validBits that are identical in the first N bits > (N = number of colors), but have different bits beyond that, and those 2 ICMs > would evaluate as equals(), but their hashcodes would be different. > > Possible solutions: > > - Truncate validBits when it is accepted in the constructor > (validBits.and(...)) > - Manually compute the hash of the first N bits of validBits > - Not use validBits in the hash code since it is allowed to omit > significant data from the hash > > (Note that everything in hashcode must participate in equals(), but > not vice versa) > > The same is true of nBits in ColorModel. (nBits can be copied and > truncated with Arrays.copyOf) > > The same is *not* true of maskBits in PCM since the constructor creates an > array of the precise length it needs... > > ...jim > > On 6/2/16 7: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@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
