Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905: ColorModel subclasses are missing hashCode() or equals() or both methods

[Philip Race]

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/ 



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/


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

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 

Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905: ColorModel subclasses are missing hashCode() or equals() or both methods

[Jayathirth D V]

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/ 

 

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/ 

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; HYPERLINK 
"mailto:2d-dev@openjdk.java.net"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/ 

Thanks, 
Jay 

-Original Message- 
From: Jim Graham 
Sent: Thursday, February 02, 2017 2:51 AM 
To: Phil Race; Jayathirth D V; HYPERLINK 

Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905: ColorModel subclasses are missing hashCode() or equals() or both methods

[Phil Race]

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@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@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 

Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905: ColorModel subclasses are missing hashCode() or equals() or both methods

[Jim Graham]

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 
ifhasAlpha 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
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@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

Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905: ColorModel subclasses are missing hashCode() or equals() or both methods

[Jayathirth D V]

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@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@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 

Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905: ColorModel subclasses are missing hashCode() or equals() or both methods

[Jayathirth D V]

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@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 

Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905: ColorModel subclasses are missing hashCode() or equals() or both methods

[Jim Graham]

I don't think there is a spec anywhere that 2 objects created by the same constructor must evaluate as equals().  In 
particular, 2 instances of Object created with the null constructor do not evaluate as equals().  So, with respect to 
testing a spec, they are reading a promise into the base class there.


On the other hand, it makes sense that similar construction would be equals() for objects intended to be compared and 
the CM subclasses have always tried to be comparable.  But, since CM is abstract you can't really test its constructors 
directly.  With these fixes we should end up having all subclasses compare as equals() for similar construction so we 
are still making good on the implied promise that they are testing here for all constructable objects.


Should we consider that CM might promise proper equals() tests for subclasses?  For one, we can't really fully promise 
that since subclasses may differ for reasons other than what CM can test, so its implementation of equals() can't be 
authoritative in general.  It is true that we used to provide a conditionally appropriate test for just the parts that 
CM was responsible for, but we never documented that.  In practice that potential promise doesn't matter for our 
internal classes, it only matters for external classes that aren't being updated here.


With regards to Phil's question about whether these changes will affect application code - since you can't instantiate 
CM directly, and since we deal with these conditions properly in all sub-classes this boils down to whether applications 
use their own subclass of CM, and I don't have a lot of data for that.  Phil?


...jim

On 1/30/17 10:37 PM, Jayathirth D V wrote:

Hi,

Regarding Jim's query of #2 JCK test case:

How do they accomplish this when the CM class is abstract?  Do they create a 
relatively empty subclass and instantiate that?
Yes they have simple subclass(MyColorModel) of ColorModel and they instantiate 
that with same "bits" and try to compare.

MyColorModel cm = new MyColorModel(4);
ColorModel x = new MyColorModel(4)
if (!cm.equals(x)) {
return Status.failed("Should return true on two instances 
instantiated by the same constructor");
}

Thanks,
Jay

-Original Message-
From: Jayathirth D V
Sent: Tuesday, January 31, 2017 11:53 AM
To: Philip Race; Jim Graham; 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

Hi Phil & Jim,

Thanks for your inputs.

I have verified with previous iteration of the changes and both #1 and #4 fail from the 
time we have introduced the change in how we calculate "nBits" in ColorModel.

-nBits = bits.clone();
+/*
+ * We need significant bits value only for the length
+ * of number of components, so we truncate remaining part.
+ * It also helps in hashCode calculation since bits[] can contain
+ * different values after the length of number of components between
+ * two ColorModels.
+ */
+nBits = Arrays.copyOf(bits, numComponents);

#2 and #3 would fail from the time we have removed equals() and hashCode() 
methods from ColorModel, to follow identity-as-equals verification.
Collectively all of them are failing right now as we have both the changes at 
http://cr.openjdk.java.net/~jdv/7107905/webrev.15/ .

Thanks,
Jay

-Original Message-
From: Phil Race
Sent: Tuesday, January 31, 2017 5:27 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

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 

Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905: ColorModel subclasses are missing hashCode() or equals() or both methods

[Jayathirth D V]

Hi Phil & Jim,

Thanks for your inputs.

I have verified with previous iteration of the changes and both #1 and #4 fail 
from the time we have introduced the change in how we calculate "nBits" in 
ColorModel.
 
-nBits = bits.clone();
+/*
+ * We need significant bits value only for the length
+ * of number of components, so we truncate remaining part.
+ * It also helps in hashCode calculation since bits[] can contain
+ * different values after the length of number of components between
+ * two ColorModels.
+ */
+nBits = Arrays.copyOf(bits, numComponents);

#2 and #3 would fail from the time we have removed equals() and hashCode() 
methods from ColorModel, to follow identity-as-equals verification.
Collectively all of them are failing right now as we have both the changes at 
http://cr.openjdk.java.net/~jdv/7107905/webrev.15/ .

Thanks,
Jay

-Original Message-
From: Phil Race 
Sent: Tuesday, January 31, 2017 5:27 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

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#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.
>
> 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
>

Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905: ColorModel subclasses are missing hashCode() or equals() or both methods

[Phil Race]

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#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.


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

Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905: ColorModel subclasses are missing hashCode() or equals() or both methods

[Jayathirth D V]

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@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@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@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 

Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905: ColorModel subclasses are missing hashCode() or equals() or both methods

[Phil Race]

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@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@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:

* 
* 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.
* 
* public boolean equals(Object obj) {
* if (!super.equals(obj)) {
* return false;
* }
* MyCMClass cm = (MyCMClass) obj;
* // test additional properties on "cm" and "this"
* }
* 


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-

Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905: ColorModel subclasses are missing hashCode() or equals() or both methods

[Jayathirth D V]

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@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@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:
>
> * 
> * 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.
> * 
> * public boolean equals(Object obj) {
> * if (!super.equals(obj)) {
> * return false;
> * }
> * MyCMClass cm = (MyCMClass) obj;
> * // test additional properties on "cm" and "this"
> * }
> * 
>
>
> 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@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 

Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905: ColorModel subclasses are missing hashCode() or equals() or both methods

[Jim Graham]

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@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:

* 
* 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.
* 
* public boolean equals(Object obj) {
* if (!super.equals(obj)) {
* return false;
* }
* MyCMClass cm = (MyCMClass) obj;
* // test additional properties on "cm" and "this"
* }
* 


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@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 

Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905: ColorModel subclasses are missing hashCode() or equals() or both methods

[Phil Race]

OK by me.

-phil.

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@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:

* 
* 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.
* 
* public boolean equals(Object obj) {
* if (!super.equals(obj)) {
* return false;
* }
* MyCMClass cm = (MyCMClass) obj;
* // test additional properties on "cm" and "this"
* }
* 


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@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 

Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905: ColorModel subclasses are missing hashCode() or equals() or both methods

[Jim Graham]

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:


* 
* 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.
* 
* public boolean equals(Object obj) {
* if (!super.equals(obj)) {
* return false;
* }
* MyCMClass cm = (MyCMClass) obj;
* // test additional properties on "cm" and "this"
* }
* 


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@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@openjdk.java.net
Subject: Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905:
ColorModel subclasses are missing hashCode() or equals() or both
methods

Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905: ColorModel subclasses are missing hashCode() or equals() or both methods

[Jim Graham]

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@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,

Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905: ColorModel subclasses are missing hashCode() or equals() or both methods

[Jayathirth D V]

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, 

Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905: ColorModel subclasses are missing hashCode() or equals() or both methods

[Jim Graham]

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

Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905: ColorModel subclasses are missing hashCode() or equals() or both methods

[Jayathirth D V]

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 

Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905: ColorModel subclasses are missing hashCode() or equals() or both methods

[Jim Graham]

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

Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905: ColorModel subclasses are missing hashCode() or equals() or both methods

[Jayathirth D V]

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

Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905: ColorModel subclasses are missing hashCode() or equals() or both methods

[Philip Race]

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

Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905: ColorModel subclasses are missing hashCode() or equals() or both methods

[Jim Graham]

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

Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905: ColorModel subclasses are missing hashCode() or equals() or both methods

[Phil Race]

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

Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905: ColorModel subclasses are missing hashCode() or equals() or both methods

[Jim Graham]

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

Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905: ColorModel subclasses are missing hashCode() or equals() or both methods

[Phil Race]

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

-phil.



...jim

On 05/29/2016 11:55 PM, Jayathirth D V wrote:

Hi Jim,

Thanks for your valuable inputs. I have made recommended changes.
Please find updated webrev for review:
http://cr.openjdk.java.net/~jdv/7107905/webrev.08/

Thanks,
Jay

-Original Message-
From: Jim Graham
Sent: Friday, May 27, 2016 11:52 PM
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 think that makes sense.  If it has no type-specific information to 
compare, then it doesn't need to override them...


...jim

On 05/27/2016 05:32 AM, Jayathirth D V wrote:

Hi Jim,

Since we will be moving comparison for ColorSpace and transferType 
to CM, can we remove equals() & hashCode() methods in CCM?
For PCM and ICM we have unique variables which we can compare, there 
will be no problem.


Thanks,
Jay

-Original Message-
From: Jim Graham
Sent: Friday, May 27, 2016 2:18 PM
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

Hi Jay,

.equals() should not be comparing any fields that are computed from 
other fields - generally fields that come directly from a 
constructor argument tend to be the fields to compare. Internal 
state variables (such as "isInited" and the like) should never be 
compared.  It should be comparing fields that affect the 
interpretation of the color.  To that end...


In CM, add comparison/hash of:
  ColorSpace
  transferType

In CCM, do not compare or hash:
  needScaleInit (internal state for lazy ScaleInit)
  noUnnorm (computed from other constructor arguments)
  nonStdScale (computed from other constructor arguments)
  signed (computed from other constructor arguments)
  transferType (tested in CM)
(basically, CCM doesn't have anything to compares as all of its
significant values are compared in the super class)

...jim

On 05/27/2016 01:02 AM, Jayathirth D V wrote:

Hi,

Gentle reminder.
Please review updated fix:
http://cr.openjdk.java.net/~jdv/7107905/webrev.07/

Thanks,
Jay

-Original Message-
From: Jayathirth D V
Sent: Thursday, May 19, 2016 6:43 PM
To: Philip Race; Jim Graham
Cc: 2d-dev@openjdk.java.net
Subject: Review Request for JDK-7107905: ColorModel subclasses are
missing hashCode() or equals() or both methods

Hi,

Previously for this bug we were making changes related only to 
IndexColorModel. Since we are expanding to include hashCode() or 
equals() method from PackedColorModel and ComponentColorModel, I 
have created single webrev for review under the same bug id.


Now the "getclass()==" check is present in base class ColorModel 
and each subclass of ColorModel have their own equality check for 
properties.


Details:
Bug : https://bugs.openjdk.java.net/browse/JDK-7107905
Updated Webrev : http://cr.openjdk.java.net/~jdv/7107905/webrev.07/

Please review the changes at your convenience.

Thanks,
Jay

Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905: ColorModel subclasses are missing hashCode() or equals() or both methods

[Phil Race]

On 05/31/2016 12:16 PM, Jim Graham wrote:

Hi Phil,

I don't think that alternate wording conveys that this is not a case 
of "instanceof" and is instead a case of "getClass() =="...


Perhaps we don't need "exact", but "type" is vague enough to call into 
question how we do the test.  I could see "the same class (and not a 
subclass) as this"...?


That wording works for me. I just thought someone else might prefer 
'type' strongly enough to ask for it.


-phil.



...jim

On 05/31/2016 11:56 AM, Phil Race wrote:


1449  * the target object must be the exact same class as this

I believe the language people like to say "the same type" so maybe say

1449  * the target object must be of the same type (i.e. class) 
as this


Could IndexColorModel cache the hash code since it could be fairly
expensive to compute ?
Arrays.hashCode() is the expensive bit.

Less so PackedColorModel but could be considered there too.

No other comments at this time.

-phil.

On 05/29/2016 11:55 PM, Jayathirth D V wrote:

Hi Jim,

Thanks for your valuable inputs. I have made recommended changes.
Please find updated webrev for review:
http://cr.openjdk.java.net/~jdv/7107905/webrev.08/

Thanks,
Jay

-Original Message-
From: Jim Graham
Sent: Friday, May 27, 2016 11:52 PM
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 think that makes sense.  If it has no type-specific information to
compare, then it doesn't need to override them...

...jim

On 05/27/2016 05:32 AM, Jayathirth D V wrote:

Hi Jim,

Since we will be moving comparison for ColorSpace and transferType to
CM, can we remove equals() & hashCode() methods in CCM?
For PCM and ICM we have unique variables which we can compare, there
will be no problem.

Thanks,
Jay

-Original Message-
From: Jim Graham
Sent: Friday, May 27, 2016 2:18 PM
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

Hi Jay,

.equals() should not be comparing any fields that are computed from
other fields - generally fields that come directly from a constructor
argument tend to be the fields to compare.  Internal state variables
(such as "isInited" and the like) should never be compared. It
should be comparing fields that affect the interpretation of the
color.  To that end...

In CM, add comparison/hash of:
  ColorSpace
  transferType

In CCM, do not compare or hash:
  needScaleInit (internal state for lazy ScaleInit)
  noUnnorm (computed from other constructor arguments)
  nonStdScale (computed from other constructor arguments)
  signed (computed from other constructor arguments)
  transferType (tested in CM)
(basically, CCM doesn't have anything to compares as all of its
significant values are compared in the super class)

...jim

On 05/27/2016 01:02 AM, Jayathirth D V wrote:

Hi,

Gentle reminder.
Please review updated fix:
http://cr.openjdk.java.net/~jdv/7107905/webrev.07/

Thanks,
Jay

-Original Message-
From: Jayathirth D V
Sent: Thursday, May 19, 2016 6:43 PM
To: Philip Race; Jim Graham
Cc: 2d-dev@openjdk.java.net
Subject: Review Request for JDK-7107905: ColorModel subclasses are
missing hashCode() or equals() or both methods

Hi,

Previously for this bug we were making changes related only to
IndexColorModel. Since we are expanding to include hashCode() or
equals() method from PackedColorModel and ComponentColorModel, I
have created single webrev for review under the same bug id.

Now the "getclass()==" check is present in base class ColorModel and
each subclass of ColorModel have their own equality check for
properties.

Details:
Bug : https://bugs.openjdk.java.net/browse/JDK-7107905
Updated Webrev : http://cr.openjdk.java.net/~jdv/7107905/webrev.07/

Please review the changes at your convenience.

Thanks,
Jay

Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905: ColorModel subclasses are missing hashCode() or equals() or both methods

[Jim Graham]

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.


Also, I'm not sure == is a good way to compare ColorSpace instances - Phil?

...jim

On 05/29/2016 11:55 PM, Jayathirth D V wrote:

Hi Jim,

Thanks for your valuable inputs. I have made recommended changes.
Please find updated webrev for review:
http://cr.openjdk.java.net/~jdv/7107905/webrev.08/

Thanks,
Jay

-Original Message-
From: Jim Graham
Sent: Friday, May 27, 2016 11:52 PM
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 think that makes sense.  If it has no type-specific information to compare, 
then it doesn't need to override them...

...jim

On 05/27/2016 05:32 AM, Jayathirth D V wrote:

Hi Jim,

Since we will be moving comparison for ColorSpace and transferType to CM, can we 
remove equals() & hashCode() methods in CCM?
For PCM and ICM we have unique variables which we can compare, there will be no 
problem.

Thanks,
Jay

-Original Message-
From: Jim Graham
Sent: Friday, May 27, 2016 2:18 PM
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

Hi Jay,

.equals() should not be comparing any fields that are computed from other fields - 
generally fields that come directly from a constructor argument tend to be the fields to 
compare.  Internal state variables (such as "isInited" and the like) should 
never be compared.  It should be comparing fields that affect the interpretation of the 
color.  To that end...

In CM, add comparison/hash of:
  ColorSpace
  transferType

In CCM, do not compare or hash:
  needScaleInit (internal state for lazy ScaleInit)
  noUnnorm (computed from other constructor arguments)
  nonStdScale (computed from other constructor arguments)
  signed (computed from other constructor arguments)
  transferType (tested in CM)
(basically, CCM doesn't have anything to compares as all of its
significant values are compared in the super class)

...jim

On 05/27/2016 01:02 AM, Jayathirth D V wrote:

Hi,

Gentle reminder.
Please review updated fix:
http://cr.openjdk.java.net/~jdv/7107905/webrev.07/

Thanks,
Jay

-Original Message-
From: Jayathirth D V
Sent: Thursday, May 19, 2016 6:43 PM
To: Philip Race; Jim Graham
Cc: 2d-dev@openjdk.java.net
Subject: Review Request for JDK-7107905: ColorModel subclasses are
missing hashCode() or equals() or both methods

Hi,

Previously for this bug we were making changes related only to IndexColorModel. 
Since we are expanding to include hashCode() or equals() method from 
PackedColorModel and ComponentColorModel, I have created single webrev for 
review under the same bug id.

Now the "getclass()==" check is present in base class ColorModel and each 
subclass of ColorModel have their own equality check for properties.

Details:
Bug : https://bugs.openjdk.java.net/browse/JDK-7107905
Updated Webrev : http://cr.openjdk.java.net/~jdv/7107905/webrev.07/

Please review the changes at your convenience.

Thanks,
Jay

Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905: ColorModel subclasses are missing hashCode() or equals() or both methods

[Jim Graham]

Hi Phil,

I don't think that alternate wording conveys that this is not a case of 
"instanceof" and is instead a case of "getClass() =="...


Perhaps we don't need "exact", but "type" is vague enough to call into 
question how we do the test.  I could see "the same class (and not a 
subclass) as this"...?


...jim

On 05/31/2016 11:56 AM, Phil Race wrote:


1449  * the target object must be the exact same class as this

I believe the language people like to say "the same type" so maybe say

1449  * the target object must be of the same type (i.e. class) as this

Could IndexColorModel cache the hash code since it could be fairly
expensive to compute ?
Arrays.hashCode() is the expensive bit.

Less so PackedColorModel but could be considered there too.

No other comments at this time.

-phil.

On 05/29/2016 11:55 PM, Jayathirth D V wrote:

Hi Jim,

Thanks for your valuable inputs. I have made recommended changes.
Please find updated webrev for review:
http://cr.openjdk.java.net/~jdv/7107905/webrev.08/

Thanks,
Jay

-Original Message-
From: Jim Graham
Sent: Friday, May 27, 2016 11:52 PM
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 think that makes sense.  If it has no type-specific information to
compare, then it doesn't need to override them...

...jim

On 05/27/2016 05:32 AM, Jayathirth D V wrote:

Hi Jim,

Since we will be moving comparison for ColorSpace and transferType to
CM, can we remove equals() & hashCode() methods in CCM?
For PCM and ICM we have unique variables which we can compare, there
will be no problem.

Thanks,
Jay

-Original Message-
From: Jim Graham
Sent: Friday, May 27, 2016 2:18 PM
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

Hi Jay,

.equals() should not be comparing any fields that are computed from
other fields - generally fields that come directly from a constructor
argument tend to be the fields to compare.  Internal state variables
(such as "isInited" and the like) should never be compared.  It
should be comparing fields that affect the interpretation of the
color.  To that end...

In CM, add comparison/hash of:
  ColorSpace
  transferType

In CCM, do not compare or hash:
  needScaleInit (internal state for lazy ScaleInit)
  noUnnorm (computed from other constructor arguments)
  nonStdScale (computed from other constructor arguments)
  signed (computed from other constructor arguments)
  transferType (tested in CM)
(basically, CCM doesn't have anything to compares as all of its
significant values are compared in the super class)

...jim

On 05/27/2016 01:02 AM, Jayathirth D V wrote:

Hi,

Gentle reminder.
Please review updated fix:
http://cr.openjdk.java.net/~jdv/7107905/webrev.07/

Thanks,
Jay

-Original Message-
From: Jayathirth D V
Sent: Thursday, May 19, 2016 6:43 PM
To: Philip Race; Jim Graham
Cc: 2d-dev@openjdk.java.net
Subject: Review Request for JDK-7107905: ColorModel subclasses are
missing hashCode() or equals() or both methods

Hi,

Previously for this bug we were making changes related only to
IndexColorModel. Since we are expanding to include hashCode() or
equals() method from PackedColorModel and ComponentColorModel, I
have created single webrev for review under the same bug id.

Now the "getclass()==" check is present in base class ColorModel and
each subclass of ColorModel have their own equality check for
properties.

Details:
Bug : https://bugs.openjdk.java.net/browse/JDK-7107905
Updated Webrev : http://cr.openjdk.java.net/~jdv/7107905/webrev.07/

Please review the changes at your convenience.

Thanks,
Jay

Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905: ColorModel subclasses are missing hashCode() or equals() or both methods

[Phil Race]

1449  * the target object must be the exact same class as this

I believe the language people like to say "the same type" so maybe say

1449  * the target object must be of the same type (i.e. class) as this

Could IndexColorModel cache the hash code since it could be fairly expensive to 
compute ?
Arrays.hashCode() is the expensive bit.

Less so PackedColorModel but could be considered there too.

No other comments at this time.

-phil.

On 05/29/2016 11:55 PM, Jayathirth D V wrote:

Hi Jim,

Thanks for your valuable inputs. I have made recommended changes.
Please find updated webrev for review:
http://cr.openjdk.java.net/~jdv/7107905/webrev.08/

Thanks,
Jay

-Original Message-
From: Jim Graham
Sent: Friday, May 27, 2016 11:52 PM
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 think that makes sense.  If it has no type-specific information to compare, 
then it doesn't need to override them...

...jim

On 05/27/2016 05:32 AM, Jayathirth D V wrote:

Hi Jim,

Since we will be moving comparison for ColorSpace and transferType to CM, can we 
remove equals() & hashCode() methods in CCM?
For PCM and ICM we have unique variables which we can compare, there will be no 
problem.

Thanks,
Jay

-Original Message-
From: Jim Graham
Sent: Friday, May 27, 2016 2:18 PM
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

Hi Jay,

.equals() should not be comparing any fields that are computed from other fields - 
generally fields that come directly from a constructor argument tend to be the fields to 
compare.  Internal state variables (such as "isInited" and the like) should 
never be compared.  It should be comparing fields that affect the interpretation of the 
color.  To that end...

In CM, add comparison/hash of:
  ColorSpace
  transferType

In CCM, do not compare or hash:
  needScaleInit (internal state for lazy ScaleInit)
  noUnnorm (computed from other constructor arguments)
  nonStdScale (computed from other constructor arguments)
  signed (computed from other constructor arguments)
  transferType (tested in CM)
(basically, CCM doesn't have anything to compares as all of its
significant values are compared in the super class)

...jim

On 05/27/2016 01:02 AM, Jayathirth D V wrote:

Hi,

Gentle reminder.
Please review updated fix:
http://cr.openjdk.java.net/~jdv/7107905/webrev.07/

Thanks,
Jay

-Original Message-
From: Jayathirth D V
Sent: Thursday, May 19, 2016 6:43 PM
To: Philip Race; Jim Graham
Cc: 2d-dev@openjdk.java.net
Subject: Review Request for JDK-7107905: ColorModel subclasses are
missing hashCode() or equals() or both methods

Hi,

Previously for this bug we were making changes related only to IndexColorModel. 
Since we are expanding to include hashCode() or equals() method from 
PackedColorModel and ComponentColorModel, I have created single webrev for 
review under the same bug id.

Now the "getclass()==" check is present in base class ColorModel and each 
subclass of ColorModel have their own equality check for properties.

Details:
Bug : https://bugs.openjdk.java.net/browse/JDK-7107905
Updated Webrev : http://cr.openjdk.java.net/~jdv/7107905/webrev.07/

Please review the changes at your convenience.

Thanks,
Jay

Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905: ColorModel subclasses are missing hashCode() or equals() or both methods

[Jayathirth D V]

Hi Jim,

Thanks for your valuable inputs. I have made recommended changes.
Please find updated webrev for review:
http://cr.openjdk.java.net/~jdv/7107905/webrev.08/ 

Thanks,
Jay 

-Original Message-
From: Jim Graham 
Sent: Friday, May 27, 2016 11:52 PM
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 think that makes sense.  If it has no type-specific information to compare, 
then it doesn't need to override them...

...jim

On 05/27/2016 05:32 AM, Jayathirth D V wrote:
> Hi Jim,
>
> Since we will be moving comparison for ColorSpace and transferType to CM, can 
> we remove equals() & hashCode() methods in CCM?
> For PCM and ICM we have unique variables which we can compare, there will be 
> no problem.
>
> Thanks,
> Jay
>
> -Original Message-
> From: Jim Graham
> Sent: Friday, May 27, 2016 2:18 PM
> 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
>
> Hi Jay,
>
> .equals() should not be comparing any fields that are computed from other 
> fields - generally fields that come directly from a constructor argument tend 
> to be the fields to compare.  Internal state variables (such as "isInited" 
> and the like) should never be compared.  It should be comparing fields that 
> affect the interpretation of the color.  To that end...
>
> In CM, add comparison/hash of:
>  ColorSpace
>  transferType
>
> In CCM, do not compare or hash:
>  needScaleInit (internal state for lazy ScaleInit)
>  noUnnorm (computed from other constructor arguments)
>  nonStdScale (computed from other constructor arguments)
>  signed (computed from other constructor arguments)
>  transferType (tested in CM)
> (basically, CCM doesn't have anything to compares as all of its 
> significant values are compared in the super class)
>
>   ...jim
>
> On 05/27/2016 01:02 AM, Jayathirth D V wrote:
>> Hi,
>>
>> Gentle reminder.
>> Please review updated fix:
>> http://cr.openjdk.java.net/~jdv/7107905/webrev.07/
>>
>> Thanks,
>> Jay
>>
>> -Original Message-
>> From: Jayathirth D V
>> Sent: Thursday, May 19, 2016 6:43 PM
>> To: Philip Race; Jim Graham
>> Cc: 2d-dev@openjdk.java.net
>> Subject: Review Request for JDK-7107905: ColorModel subclasses are 
>> missing hashCode() or equals() or both methods
>>
>> Hi,
>>
>> Previously for this bug we were making changes related only to 
>> IndexColorModel. Since we are expanding to include hashCode() or equals() 
>> method from PackedColorModel and ComponentColorModel, I have created single 
>> webrev for review under the same bug id.
>>
>> Now the "getclass()==" check is present in base class ColorModel and each 
>> subclass of ColorModel have their own equality check for properties.
>>
>> Details:
>> Bug : https://bugs.openjdk.java.net/browse/JDK-7107905
>> Updated Webrev : http://cr.openjdk.java.net/~jdv/7107905/webrev.07/
>>
>> Please review the changes at your convenience.
>>
>> Thanks,
>> Jay
>>

Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905: ColorModel subclasses are missing hashCode() or equals() or both methods

[Jim Graham]

I think that makes sense.  If it has no type-specific information to 
compare, then it doesn't need to override them...


...jim

On 05/27/2016 05:32 AM, Jayathirth D V wrote:

Hi Jim,

Since we will be moving comparison for ColorSpace and transferType to CM, can we 
remove equals() & hashCode() methods in CCM?
For PCM and ICM we have unique variables which we can compare, there will be no 
problem.

Thanks,
Jay

-Original Message-
From: Jim Graham
Sent: Friday, May 27, 2016 2:18 PM
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

Hi Jay,

.equals() should not be comparing any fields that are computed from other fields - 
generally fields that come directly from a constructor argument tend to be the fields to 
compare.  Internal state variables (such as "isInited" and the like) should 
never be compared.  It should be comparing fields that affect the interpretation of the 
color.  To that end...

In CM, add comparison/hash of:
 ColorSpace
 transferType

In CCM, do not compare or hash:
 needScaleInit (internal state for lazy ScaleInit)
 noUnnorm (computed from other constructor arguments)
 nonStdScale (computed from other constructor arguments)
 signed (computed from other constructor arguments)
 transferType (tested in CM)
(basically, CCM doesn't have anything to compares as all of its significant 
values are compared in the super class)

...jim

On 05/27/2016 01:02 AM, Jayathirth D V wrote:

Hi,

Gentle reminder.
Please review updated fix:
http://cr.openjdk.java.net/~jdv/7107905/webrev.07/

Thanks,
Jay

-Original Message-
From: Jayathirth D V
Sent: Thursday, May 19, 2016 6:43 PM
To: Philip Race; Jim Graham
Cc: 2d-dev@openjdk.java.net
Subject: Review Request for JDK-7107905: ColorModel subclasses are
missing hashCode() or equals() or both methods

Hi,

Previously for this bug we were making changes related only to IndexColorModel. 
Since we are expanding to include hashCode() or equals() method from 
PackedColorModel and ComponentColorModel, I have created single webrev for 
review under the same bug id.

Now the "getclass()==" check is present in base class ColorModel and each 
subclass of ColorModel have their own equality check for properties.

Details:
Bug : https://bugs.openjdk.java.net/browse/JDK-7107905
Updated Webrev : http://cr.openjdk.java.net/~jdv/7107905/webrev.07/

Please review the changes at your convenience.

Thanks,
Jay

Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905: ColorModel subclasses are missing hashCode() or equals() or both methods

[Jayathirth D V]

Hi Jim,

Since we will be moving comparison for ColorSpace and transferType to CM, can 
we remove equals() & hashCode() methods in CCM? 
For PCM and ICM we have unique variables which we can compare, there will be no 
problem.

Thanks,
Jay

-Original Message-
From: Jim Graham 
Sent: Friday, May 27, 2016 2:18 PM
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

Hi Jay,

.equals() should not be comparing any fields that are computed from other 
fields - generally fields that come directly from a constructor argument tend 
to be the fields to compare.  Internal state variables (such as "isInited" and 
the like) should never be compared.  It should be comparing fields that affect 
the interpretation of the color.  To that end...

In CM, add comparison/hash of:
ColorSpace
transferType

In CCM, do not compare or hash:
needScaleInit (internal state for lazy ScaleInit)
noUnnorm (computed from other constructor arguments)
nonStdScale (computed from other constructor arguments)
signed (computed from other constructor arguments)
transferType (tested in CM)
(basically, CCM doesn't have anything to compares as all of its significant 
values are compared in the super class)

...jim

On 05/27/2016 01:02 AM, Jayathirth D V wrote:
> Hi,
>
> Gentle reminder.
> Please review updated fix:
> http://cr.openjdk.java.net/~jdv/7107905/webrev.07/
>
> Thanks,
> Jay
>
> -Original Message-
> From: Jayathirth D V
> Sent: Thursday, May 19, 2016 6:43 PM
> To: Philip Race; Jim Graham
> Cc: 2d-dev@openjdk.java.net
> Subject: Review Request for JDK-7107905: ColorModel subclasses are 
> missing hashCode() or equals() or both methods
>
> Hi,
>
> Previously for this bug we were making changes related only to 
> IndexColorModel. Since we are expanding to include hashCode() or equals() 
> method from PackedColorModel and ComponentColorModel, I have created single 
> webrev for review under the same bug id.
>
> Now the "getclass()==" check is present in base class ColorModel and each 
> subclass of ColorModel have their own equality check for properties.
>
> Details:
> Bug : https://bugs.openjdk.java.net/browse/JDK-7107905
> Updated Webrev : http://cr.openjdk.java.net/~jdv/7107905/webrev.07/
>
> Please review the changes at your convenience.
>
> Thanks,
> Jay
>

Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905: ColorModel subclasses are missing hashCode() or equals() or both methods

[Jim Graham]

Hi Jay,

.equals() should not be comparing any fields that are computed from 
other fields - generally fields that come directly from a constructor 
argument tend to be the fields to compare.  Internal state variables 
(such as "isInited" and the like) should never be compared.  It should 
be comparing fields that affect the interpretation of the color.  To 
that end...


In CM, add comparison/hash of:
   ColorSpace
   transferType

In CCM, do not compare or hash:
   needScaleInit (internal state for lazy ScaleInit)
   noUnnorm (computed from other constructor arguments)
   nonStdScale (computed from other constructor arguments)
   signed (computed from other constructor arguments)
   transferType (tested in CM)
(basically, CCM doesn't have anything to compares as all of its 
significant values are compared in the super class)


...jim

On 05/27/2016 01:02 AM, Jayathirth D V wrote:

Hi,

Gentle reminder.
Please review updated fix:
http://cr.openjdk.java.net/~jdv/7107905/webrev.07/

Thanks,
Jay

-Original Message-
From: Jayathirth D V
Sent: Thursday, May 19, 2016 6:43 PM
To: Philip Race; Jim Graham
Cc: 2d-dev@openjdk.java.net
Subject: Review Request for JDK-7107905: ColorModel subclasses are missing 
hashCode() or equals() or both methods

Hi,

Previously for this bug we were making changes related only to IndexColorModel. 
Since we are expanding to include hashCode() or equals() method from 
PackedColorModel and ComponentColorModel, I have created single webrev for 
review under the same bug id.

Now the "getclass()==" check is present in base class ColorModel and each 
subclass of ColorModel have their own equality check for properties.

Details:
Bug : https://bugs.openjdk.java.net/browse/JDK-7107905
Updated Webrev : http://cr.openjdk.java.net/~jdv/7107905/webrev.07/

Please review the changes at your convenience.

Thanks,
Jay

Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905: ColorModel subclasses are missing hashCode() or equals() or both methods

[Jayathirth D V]

Hi,

Gentle reminder.
Please review updated fix:
http://cr.openjdk.java.net/~jdv/7107905/webrev.07/

Thanks,
Jay

-Original Message-
From: Jayathirth D V 
Sent: Thursday, May 19, 2016 6:43 PM
To: Philip Race; Jim Graham
Cc: 2d-dev@openjdk.java.net
Subject: Review Request for JDK-7107905: ColorModel subclasses are missing 
hashCode() or equals() or both methods

Hi,

Previously for this bug we were making changes related only to IndexColorModel. 
Since we are expanding to include hashCode() or equals() method from 
PackedColorModel and ComponentColorModel, I have created single webrev for 
review under the same bug id.

Now the "getclass()==" check is present in base class ColorModel and each 
subclass of ColorModel have their own equality check for properties.

Details: 
Bug : https://bugs.openjdk.java.net/browse/JDK-7107905 
Updated Webrev : http://cr.openjdk.java.net/~jdv/7107905/webrev.07/ 

Please review the changes at your convenience.

Thanks,
Jay