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@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@openjdk.java.net
<mailto: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
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@openjdk.java.net
<mailto: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
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
