[OpenJDK 2D-Dev] Review Request for JDK-7107905: ColorModel subclasses are missing hashCode() or equals() or both methods
Jayathirth D V
jayathirth.d.v at oracle.com
Mon Jan 30 10:22:07 UTC 2017
Hi Phil,
Thanks for your review, I have updated the webrev to include your suggestions :
http://cr.openjdk.java.net/~jdv/7107905/webrev.15/
I just noticed that some JCK test are failing because of this change :
1)api/java_awt/Image/ColorModel/index.html#Constructor: Failed. test cases: 4; passed: 3; failed: 1; first test case failure: ColorModel2001
This test fails because getComponentSize() returned an array with length 3 but it expects the length to be 4. In the test case they have bits per component array of length 4 like {8, 8, 8, 8}. But in the test case wherever they are passing "has Alpha" as "false" we omit the alpha component bit. This is because of tighter check that we have in ColorModel class as "nBits = Arrays.copyOf(bits, numComponents);" . "numComponents" will be 3 if hasAlpha is false.
2)api/java_awt/Image/ColorModel/index.html#Equals: Failed. test cases: 3; passed: 2; failed: 1; first test case failure: ColorModel0004
Here they check for equality between 2 ColorModel objects having same values, but it fails because now we are using identity-as-equals check in ColorModel.
3)api/java_awt/Image/ColorModel/index.html#HashCode: Failed. test cases: 2; passed: 1; failed: 1; first test case failure: ColorModel2006
Here they check for hashCode equality between 2 ColorModel objects having same values, but it fails since we don't have hashCode check in ColorModel and it will be different between 2 Objects.
4)api/java_awt/Image/ComponentColorModel/index.html#ConstructorTesttestCase1: Failed. test cases: 2; passed: 1; failed: 1; first test case failure: testCase1
Throws "java.lang.ArrayIndexOutOfBoundsException: 3". This is also happening because of same reason as why the first JCK test is failing. We omit alpha bit if hasAlpha is false but JCK test tries to call getComponentSize() with index 3 which throws ArrayIndexOutOfBoundsException.
Please provide your inputs.
Thanks,
Jay
-----Original Message-----
From: Phil Race
Sent: Friday, January 27, 2017 10:12 PM
To: Jayathirth D V; 2d-dev at openjdk.java.net
Subject: Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905: ColorModel subclasses are missing hashCode() or equals() or both methods
Hi Jay,
Joe had suggested
>ColorModel is redefined to use identity-is-equality equals semantics, the default behavior from object.
> This avoids the need to define a hashcode method in the base class.
However you have restored the problem that started this.
ColorModel now over-rides equals but not hashCode and it doesn't matter that it just delegates.
Code that checks for this pattern can't tell that.
I think Joe must have meant to *remove* the ColorModel.equals() method entirely.
Other than that it looks fine to me.
-phil.
On 01/19/2017 02:17 AM, Jayathirth D V wrote:
> Hello All,
>
> After getting feedback from Joe in CCC I have made changes to the code to reflect Joe's suggestions.
>
> For ColorModel class I have just added identity-is-equality equals().
> For its subclasses like IndexColorModel, PackedColorModel & ComponentColorModel, I have added instanceOf checks and checking all fields for equality. Since we are not checking any fields in ColorModel, in subclasses there will be redundant checks.
> DirectColorModel has no unique properties to check so it will be using PackedColorModel equals() and hashCode().
>
> Please review the updated code at your convenience:
> http://cr.openjdk.java.net/~jdv/7107905/webrev.14/
>
> Thanks,
> Jay
>
> -----Original Message-----
> From: Jim Graham
> Sent: Friday, July 22, 2016 3:15 AM
> To: Jayathirth D V; Philip Race
> Cc: 2d-dev at openjdk.java.net
> Subject: Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905:
> ColorModel subclasses are missing hashCode() or equals() or both
> methods
>
> Looks good to me...
>
> ...jim
>
> On 07/20/2016 09:53 AM, Jayathirth D V wrote:
>> Hi Jim,
>>
>> Thanks for your input.
>> I have updated specification of ColorModel and its subclasses.
>> Please find new webrev for review:
>> http://cr.openjdk.java.net/~jdv/7107905/webrev.13/
>>
>> Thanks,
>> Jay
>>
>> -----Original Message-----
>> From: Jim Graham
>> Sent: Tuesday, July 12, 2016 7:41 PM
>> To: Jayathirth D V; Philip Race
>> Cc: 2d-dev at openjdk.java.net
>> Subject: Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905:
>> ColorModel subclasses are missing hashCode() or equals() or both
>> methods
>>
>> Hi Jay,
>>
>> When I write javadoc suggestions I sometimes use the short-hand "{some text}" to indicate that it should be formatted using javadoc protocols, typically that expands to "{@code some text}", but sometimes a link may be appropriate. It looks like you copied and pasted a number of places where I wrote "{foo}" in email and just left the bare braces there, please adjust those (most of them should probably just have "@code" inserted.
>>
>> In ColorModel.equals(), I'd replace the entire "following properties" sentence with:
>>
>> * Subclasses may check additional properties, but this method
>> * will check the following basic properties for equivalence to
>> * determine if the target object equals this object:
>>
>> The text for IndexColorModel is also a little odd. I'd change it to something like:
>>
>> * The target object and this object will be equal iff
>> * {@link ColorModel#equals()} returns true and the following
>> * properties are also the same:
>>
>> For Packed:
>>
>> * The target object and this object will be equal iff
>> * {@link ColorModel#equals()} returns true and the masks
>> * of the color and alpha samples are the same.
>>
>> In terms of Joe's request, I'd add the following text to ColorModel.equals() right after we talk about all of the properties that it checks:
>>
>> * <p>
>> * Subclasses should override this method if they have any additional
>> * properties to compare and should use the following implementation.
>> * Note that the base {@code ColorModel} class already ensures that
>> * the target object is the same class as this object so the cast to
>> * the subclass type can be assumed if {@code super.equals(obj)}
>> returns
>> * true.
>> * <pre>
>> * public boolean equals(Object obj) {
>> * if (!super.equals(obj)) {
>> * return false;
>> * }
>> * MyCMClass cm = (MyCMClass) obj;
>> * // test additional properties on "cm" and "this"
>> * }
>> * </pre>
>>
>>
>> On 7/1/16 3:31 AM, Jayathirth D V wrote:
>>> Hi Jim,
>>>
>>> Thanks for your inputs.
>>> I have discussed with Phil also regarding the same issue offline.
>>> I have collated all the changes mentioned by you and Phil in the latest webrev:
>>> http://cr.openjdk.java.net/~jdv/7107905/webrev.12/
>>>
>>> But I was not able to understand your statement - "Arguably, we could omit the class comparison text from the subclasses as well by using a reference like that, but I think that the class equivalence test is enough out of the ordinary that it does bear reiterating it in every subclass as you already do now even though we only reference the specific other properties tested by a reference.", please clarify.
>>>
>>> Also I am not able find a way to describe Joe's concern of "The ColorModel equals and hashCode methods should spell out the protocol subclasses need to follow to obey the respective contracts of methods.", I have discussed with Phil also regarding the same offline. Any inputs on this also would be helpful.
>>>
>>> Thanks,
>>> Jay
>>>
>>> -----Original Message-----
>>> From: Jim Graham
>>> Sent: Thursday, June 30, 2016 4:05 AM
>>> To: Jayathirth D V; Philip Race; 2d-dev at openjdk.java.net
>>> Subject: Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905:
>>> ColorModel subclasses are missing hashCode() or equals() or both
>>> methods
>>>
>>> Hi Jay,
>>>
>>> We need to reference the properties from the base class in the subclasses. We don't need to list them every time, but we could introduce the "additional properties" using something like:
>>>
>>> "... we check all of the properties checked by the {equals} method of {ColorModel}, and the following additional properties:"
>>>
>>> Arguably, we could omit the class comparison text from the subclasses as well by using a reference like that, but I think that the class equivalence test is enough out of the ordinary that it does bear reiterating it in every subclass as you already do now even though we only reference the specific other properties tested by a reference.
>>>
>>> A few grammar fixes:
>>>
>>> In all of the classes, insert a space before any parentheses in
>>> comment text as in "the same class (and not a subclass)". (That
>>> wouldn't apply if the text in the comment is referring to code -
>>> then space it as you would actual code.)
>>>
>>> In CM, insert the word "the" as in "also check the following ..."
>>>
>>> In ICM I would refer to the "Valid bits of" instead as "The list of valid pixel indices in".
>>>
>>> In PCM I would change "check maskArray of ..." to "check the masks of the ..." and pluralize the word "samples".
>>>
>>> ...jim
>>>
>>> On 06/29/2016 04:13 AM, Jayathirth D V wrote:
>>>> 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 at 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 at 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 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
More information about the 2d-dev
mailing list