[OpenJDK 2D-Dev] Review Request for JDK-7107905: ColorModel subclasses are missing hashCode() or equals() or both methods
Phil Race
philip.race at oracle.com
Thu Feb 9 22:05:24 UTC 2017
Oh .. my reply was to an off-list email. I did not notice that.
So I should repeat that here :
On 2/9/17 12:38 PM, Phil Race wrote:
> 32 import java.util.Objects;
>
> This is now un-used, isn't it ? Yet all 3 subclasses still have this
> import.
>
> I don't need to "approve" a new webrev containing that but it would
> be good to publish one.
>
> +1
-phil.
On 02/09/2017 01:59 PM, Jim Graham wrote:
> From my end this looks good. +1 except for 2 outstanding review issues:
>
> - Would like to hear back final comments from Joe Darcy on the new doc
> changes/CCC request
> - Phil pointed out that there is an unneeded import in some of the
> files. I agree that we should make a final webrev to delete them, but
> I don't need to approve it if that is the only change...
>
> ...jim
>
> On 2/8/17 11:56 PM, Jayathirth D V wrote:
>> Hello All,
>>
>> There was a closed test which was failing because of
>> identity-as-equals approach for ColorModel equals() method.
>> I have modified it and added in the webrev. Along with this we are
>> now using colorspace.hashCode() in hashCode() functions instead of
>> Objects.hashCode(this.colorspace). Reverted using Arrays.equals() in
>> IndexColorModel equals() method because Arrays.copyOf() takes lot of
>> time.
>>
>> Please find updated webrev for review :
>> http://cr.openjdk.java.net/~jdv/7107905/webrev.18/
>>
>> Ran jtreg test and JCK there are no additional test case failures
>> because of the above change. Only 4 JCK tests are failing as it was
>> happening previously.
>>
>> Just copy pasted my observation regarding JCK failures so that we can
>> trace it easily:
>>
>> 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.
>>
>> Thanks,
>> Jay
>> -----Original Message-----
>> From: Jayathirth D V
>> Sent: Wednesday, February 08, 2017 3:41 PM
>> To: Jim Graham; 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
>>
>> Hello All,
>>
>> I have updated the webrev to include the following changes.
>>
>> 1) Have identity as equals check in equals() method of ColorModel
>> but elaborate the specification of equals() and hashCode() in
>> ColorModel on what properties to check in subclasses of
>> ColorModel.
>> 2) Made changes to test case to have single helper method
>> wherever we have same equals/hashCode() check.
>> 3) Updated IndexColorModel equals() method to use Arrays.equals()
>> for rgb[] data.
>> 4) Add comment on why we are not using validBits to calculate
>> hashCode() in IndexColorModel hashCode() method.
>>
>> Please find updated webrev for review :
>> http://cr.openjdk.java.net/~jdv/7107905/webrev.17/
>>
>> Thanks,
>> Jay
>>
>> -----Original Message-----
>> From: Jim Graham
>> Sent: Thursday, February 02, 2017 2:51 AM
>> To: Phil Race; 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
>>
>> I think we should move this issue (array size returned from
>> getCompSizes) into a separate bug entry and a separate fix.
>> I don't think we need to fix the clone() in the constructor and the
>> getter just to get hashcode/equals right...
>>
>> ...jim
>>
>> On 1/31/17 2:34 PM, Jim Graham wrote:
>>> For an application to run into this same issue they'd have to expect
>>> getCompSizes() to return data for components that don't exist. It's
>>> unlikely they would use that data if they really understand the
>>> objects. While that would be odd, I guess I can see someone might be
>>> constructing all of their CM's from an array of 4 components
>>> regardless of the number of actual components and we'd be happily
>>> remembering the useless extra components and returning an array of 4
>>> from getCompSizes(). As I said, they shouldn't really be reading and
>>> interpreting those extra components for any image processing, but I
>>> can imagine that they might do something like create a variant CM by
>>> calling the CompSizes() and copying them into a new array to
>>> construct a new CM with modifications. They might just robotically
>>> always copy 4 values without really checking how many are valid.
>>> That's a stretch, and their code is weak. I can conceive of how
>>> this might happen, but I really have no idea how likely it is...
>>>
>>> ...jim
>>>
>>> On 1/30/17 3:56 PM, Phil Race wrote:
>>>> Sounds like we should at least try to get the tests updated so they
>>>> only test what the spec. says.
>>>> Although it does indicate that there is at least a chance that
>>>> application code might also fail due to similar assumptions.
>>>> Does #1 not fail with the previous iteration of this change too ?
>>>>
>>>> -phil.
>>>>
>>>> On 01/30/2017 01:40 PM, Jim Graham wrote:
>>>>> Hmmm. Sounds like the test cases were written based on bugs in the
>>>>> implementation. I'm not sure what the best tactic is here for the
>>>>> short term for getting this in, but many of these changes should
>>>>> eventually be considered bugs in the tests. Is it acceptable to
>>>>> break API tests like this at the last minute even if the tests are
>>>>> at fault? Phil?
>>>>>
>>>>> Notes on specific instances below...
>>>>>
>>>>> On 1/30/17 2:22 AM, Jayathirth D V wrote:
>>>>>> Hi Phil,
>>>>>>
>>>>>> 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.
>>>>>
>>>>> This is a bug in the test then, especially if the size of our
>>>>> array matches the return value of getNumComponents.
>>>>>
>>>>>> 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.
>>>>>
>>>>> How do they accomplish this when the CM class is abstract? Do they
>>>>> create a relatively empty subclass and instantiate that?
>>>>>
>>>>> The documentation for the equals() method does not document the
>>>>> conditions under which it returns true, it uses a vague concept of
>>>>> "equals this ColorModel". I don't see how they could test
>>>>> anything given that documentation.
>>>>>
>>>>>> 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.
>>>>>
>>>>> Same as above, there are no promises documented.
>>>>>
>>>>>>
>>>>>> 4)api/java_awt/Image/ComponentColorModel/index.html#ConstructorTest
>>>>>> testCase1: 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.
>>>>>
>>>>> Same assessment as #1 above...
>>>>>
>>>>> Again, while these are my recommendations about the correctness of
>>>>> these tests, the question remains whether we want to introduce a
>>>>> change at this point in the release cycle that will essentially
>>>>> invalidate a number of tests that have been working for several
>>>>> releases already. I'll leave that tactic issue to Phil...
>>>>>
>>>>> ...jim
>>>>>
>>>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/2d-dev/attachments/20170209/c71a98ce/attachment.html>
More information about the 2d-dev
mailing list