[OpenJDK 2D-Dev] Review Request for JDK-7107905: ColorModel subclasses are missing hashCode() or equals() or both methods
Philip Race
philip.race at oracle.com
Fri Feb 10 05:12:54 UTC 2017
So +1 .. and please update the CCC
-phil.
On 2/9/17, 8:51 PM, Jayathirth D V wrote:
>
> Hi,
>
> Phil & Jim thanks for your review.
>
> I have modified the code to remove the unneeded import of import
> java.util.Objects.
>
> Please find the updated webrev :
>
> http://cr.openjdk.java.net/~jdv/7107905/webrev.19/
> <http://cr.openjdk.java.net/%7Ejdv/7107905/webrev.19/>
>
> Thanks,
>
> Jay
>
> *From:*Phil Race
> *Sent:* Friday, February 10, 2017 3:35 AM
> *To:* Jim Graham; 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
>
> 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/
> <http://cr.openjdk.java.net/%7Ejdv/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
> <mailto: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/
> <http://cr.openjdk.java.net/%7Ejdv/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
> <mailto: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/ac17d484/attachment.html>
More information about the 2d-dev
mailing list