Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905: equals() method in IndexColorModel doesnt exist and it relies on ColorModel.equals() which is not strict
+1.
-phil.
On 04/29/2016 02:35 AM, Jayathirth D V wrote:
Hi Jim,
Thanks for the approval.
I have not changed things in test case.
Regards,
Jay
-Original Message-
From: Jim Graham
Sent: Friday, April 29, 2016 2:33 PM
To: Jayathirth D V; Philip Race
Cc: 2d-dev@openjdk.java.net
Subject: Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905: equals() method
in IndexColorModel doesnt exist and it relies on ColorModel.equals() which is
not strict
That looks fine. I assumed that the test case hadn't changed since last time
and didn't read through it. Let me know if I need to look at anything in that
file...
...jim
On 04/29/2016 01:21 AM, Jayathirth D V wrote:
Hi Jim,
Thanks for the review.
I have updated the changes mentioned by you. ICM.hashCode() was generated using NetBeans
and I added super.hashCode() to include things from ColorModel. Initializing
"hash" to 7 is redundant so I removed it and initialized to super.hashCode().
Please find updated webrev for review :
http://cr.openjdk.java.net/~jdv/7107905/webrev.06/
I will make changes in CCC after technical review is approved.
Thanks,
Jay
-Original Message-
From: Jim Graham
Sent: Wednesday, April 27, 2016 4:24 AM
To: Jayathirth D V; Philip Race
Cc: 2d-dev@openjdk.java.net
Subject: Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905: equals()
method in IndexColorModel doesnt exist and it relies on
ColorModel.equals() which is not strict
This version of the ICM.equals() method starts with casting the unkown obj to ICM, which may throw
a cast exception. That cast needs to be moved down after the call to super.equals() ("this ==
cm" can just be "this == obj").
In the ICM hashcode function, is there a reason that the hash variable isn't
just initialized to super.hashCode() rather than starting with 7 and doing a
multiply-add to incorporate the super hashcode?
...jim
On 4/25/16 1:26 AM, Jayathirth D V wrote:
Hi,
Based on Joe's and Jim's suggestion I have made changes to include check for
getClass() in baseclass and use super.equals() from subclasses to verify class
equality.
Please find updated webrev for review :
http://cr.openjdk.java.net/~jdv/7107905/webrev.05/
For JDK-8153943 I will make relevant changes in its subclasses and sent review
in its thread.
Thanks,
Jay
-Original Message-
From: Jim Graham
Sent: Thursday, April 14, 2016 2:57 AM
To: Jayathirth D V; Philip Race
Cc: 2d-dev@openjdk.java.net
Subject: Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905:
equals() method in IndexColorModel doesnt exist and it relies on
ColorModel.equals() which is not strict
Looks good. The only thing left is the CCC...
...jim
On 4/13/16 1:33 AM, Jayathirth D V wrote:
Hi,
Thanks Phil & Jim for your suggestion.
I have made changes mentioned by Jim for whether we have validate individual
bits of validBits or not in different conditions.
Please find updated webrev for review :
http://cr.openjdk.java.net/~jdv/7107905/webrev.04/
Thanks,
Jay
-Original Message-
From: Phil Race
Sent: Wednesday, April 13, 2016 1:49 AM
To: Jim Graham
Cc: Jayathirth D V; Prasanta Sadhukhan; 2d-dev@openjdk.java.net
Subject: Re: Review Request for JDK-7107905: equals() method in
IndexColorModel doesnt exist and it relies on ColorModel.equals()
which is not strict
Seems like this would work/help.
-phil.
On 04/12/2016 01:19 PM, Jim Graham wrote:
On 4/12/2016 12:59 PM, Phil Race wrote:
I am catching up on email here, and "+1" but a couple of comments
- I suppose that we can't learn anything useful from
"cm.validbits.equals(this.validbits)"
since only the bits up to "map_size" should be tested ?
Perhaps if the constructors truncated it at the required size we
could use equals(). I'm not sure that is worthwhile, given how
rarely it is used. I think it is used by default on some platforms (Windows?
X11?) if they have an 8-bit screen with a sparse colormap, but that
should be pretty rare these days since nearly everything we use
should be 24-bits these days.
I have a better idea, though.
But, since the field is assigned the original supplied value from
the constructor, then the likelihood that a non-null value will be
== identical to another colormap is likely larger than normal,
perhaps we can make it faster by checking for == and then slipping
into the faster test that simply compares the rgb[] values? Something like:
if (validBits == cm.validBits) {
compare rgb[] entries
} else if (validBits == null || cm.validBits == null) {
return false;
} else {
bigger loop that compares rgb[] and validBits() values }
Note that if the two fields are == and non-null, then the entries
in the rgb[] array for indices that are non-valid should have zeros
in them because of the way that the colormap data is copied
internally, so the rgb[] comparisons should be valid without
checking the associated bits.
Potentially we could also use:
boolean fulltest;
if (validBits ==
Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905: equals() method in IndexColorModel doesnt exist and it relies on ColorModel.equals() which is not strict
This version of the ICM.equals() method starts with casting the unkown
obj to ICM, which may throw a cast exception. That cast needs to be
moved down after the call to super.equals() ("this == cm" can just be
"this == obj").
In the ICM hashcode function, is there a reason that the hash variable
isn't just initialized to super.hashCode() rather than starting with 7
and doing a multiply-add to incorporate the super hashcode?
...jim
On 4/25/16 1:26 AM, Jayathirth D V wrote:
Hi,
Based on Joe's and Jim's suggestion I have made changes to include check for
getClass() in baseclass and use super.equals() from subclasses to verify class
equality.
Please find updated webrev for review :
http://cr.openjdk.java.net/~jdv/7107905/webrev.05/
For JDK-8153943 I will make relevant changes in its subclasses and sent review
in its thread.
Thanks,
Jay
-Original Message-
From: Jim Graham
Sent: Thursday, April 14, 2016 2:57 AM
To: Jayathirth D V; Philip Race
Cc: 2d-dev@openjdk.java.net
Subject: Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905: equals() method
in IndexColorModel doesnt exist and it relies on ColorModel.equals() which is
not strict
Looks good. The only thing left is the CCC...
...jim
On 4/13/16 1:33 AM, Jayathirth D V wrote:
Hi,
Thanks Phil & Jim for your suggestion.
I have made changes mentioned by Jim for whether we have validate individual
bits of validBits or not in different conditions.
Please find updated webrev for review :
http://cr.openjdk.java.net/~jdv/7107905/webrev.04/
Thanks,
Jay
-Original Message-
From: Phil Race
Sent: Wednesday, April 13, 2016 1:49 AM
To: Jim Graham
Cc: Jayathirth D V; Prasanta Sadhukhan; 2d-dev@openjdk.java.net
Subject: Re: Review Request for JDK-7107905: equals() method in
IndexColorModel doesnt exist and it relies on ColorModel.equals()
which is not strict
Seems like this would work/help.
-phil.
On 04/12/2016 01:19 PM, Jim Graham wrote:
On 4/12/2016 12:59 PM, Phil Race wrote:
I am catching up on email here, and "+1" but a couple of comments
- I suppose that we can't learn anything useful from
"cm.validbits.equals(this.validbits)"
since only the bits up to "map_size" should be tested ?
Perhaps if the constructors truncated it at the required size we
could use equals(). I'm not sure that is worthwhile, given how
rarely it is used. I think it is used by default on some platforms (Windows?
X11?) if they have an 8-bit screen with a sparse colormap, but that
should be pretty rare these days since nearly everything we use
should be 24-bits these days.
I have a better idea, though.
But, since the field is assigned the original supplied value from the
constructor, then the likelihood that a non-null value will be ==
identical to another colormap is likely larger than normal, perhaps
we can make it faster by checking for == and then slipping into the
faster test that simply compares the rgb[] values? Something like:
if (validBits == cm.validBits) {
compare rgb[] entries
} else if (validBits == null || cm.validBits == null) {
return false;
} else {
bigger loop that compares rgb[] and validBits() values }
Note that if the two fields are == and non-null, then the entries in
the rgb[] array for indices that are non-valid should have zeros in
them because of the way that the colormap data is copied internally,
so the rgb[] comparisons should be valid without checking the
associated bits.
Potentially we could also use:
boolean fulltest;
if (validBits == cm.validBits) {
fulltest = false;
} else if (validBits == null || cm.validBits == null) {
return false;
} else if (validBits.equals(cm.validBits)) {
fulltest = false;
} else {
fulltest = true;
}
if (fulltest) {
compare both rgb[] and validBits() values } else {
compare rgb[] entries alone
}
In this case we are using validBits.equals() to reduce the amount of
testing in the loop, but are not basing a conclusion on whether the
result of the comparison will be true or false. equals() implies we
don't need to compare its values bit by bit, but !equals() doesn't
mean that the ICMs aren't equals()...
...jim
Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905: equals() method in IndexColorModel doesnt exist and it relies on ColorModel.equals() which is not strict
Hi,
Based on Joe's and Jim's suggestion I have made changes to include check for
getClass() in baseclass and use super.equals() from subclasses to verify class
equality.
Please find updated webrev for review :
http://cr.openjdk.java.net/~jdv/7107905/webrev.05/
For JDK-8153943 I will make relevant changes in its subclasses and sent review
in its thread.
Thanks,
Jay
-Original Message-
From: Jim Graham
Sent: Thursday, April 14, 2016 2:57 AM
To: Jayathirth D V; Philip Race
Cc: 2d-dev@openjdk.java.net
Subject: Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905: equals() method
in IndexColorModel doesnt exist and it relies on ColorModel.equals() which is
not strict
Looks good. The only thing left is the CCC...
...jim
On 4/13/16 1:33 AM, Jayathirth D V wrote:
> Hi,
>
> Thanks Phil & Jim for your suggestion.
> I have made changes mentioned by Jim for whether we have validate individual
> bits of validBits or not in different conditions.
> Please find updated webrev for review :
> http://cr.openjdk.java.net/~jdv/7107905/webrev.04/
>
> Thanks,
> Jay
>
> -Original Message-
> From: Phil Race
> Sent: Wednesday, April 13, 2016 1:49 AM
> To: Jim Graham
> Cc: Jayathirth D V; Prasanta Sadhukhan; 2d-dev@openjdk.java.net
> Subject: Re: Review Request for JDK-7107905: equals() method in
> IndexColorModel doesnt exist and it relies on ColorModel.equals()
> which is not strict
>
> Seems like this would work/help.
>
> -phil.
>
> On 04/12/2016 01:19 PM, Jim Graham wrote:
>>
>>
>> On 4/12/2016 12:59 PM, Phil Race wrote:
>>> I am catching up on email here, and "+1" but a couple of comments
>>>
>>> - I suppose that we can't learn anything useful from
>>> "cm.validbits.equals(this.validbits)"
>>> since only the bits up to "map_size" should be tested ?
>>
>> Perhaps if the constructors truncated it at the required size we
>> could use equals(). I'm not sure that is worthwhile, given how
>> rarely it is used. I think it is used by default on some platforms (Windows?
>> X11?) if they have an 8-bit screen with a sparse colormap, but that
>> should be pretty rare these days since nearly everything we use
>> should be 24-bits these days.
>>
>> I have a better idea, though.
>>
>> But, since the field is assigned the original supplied value from the
>> constructor, then the likelihood that a non-null value will be ==
>> identical to another colormap is likely larger than normal, perhaps
>> we can make it faster by checking for == and then slipping into the
>> faster test that simply compares the rgb[] values? Something like:
>>
>> if (validBits == cm.validBits) {
>> compare rgb[] entries
>> } else if (validBits == null || cm.validBits == null) {
>> return false;
>> } else {
>> bigger loop that compares rgb[] and validBits() values }
>>
>> Note that if the two fields are == and non-null, then the entries in
>> the rgb[] array for indices that are non-valid should have zeros in
>> them because of the way that the colormap data is copied internally,
>> so the rgb[] comparisons should be valid without checking the
>> associated bits.
>>
>> Potentially we could also use:
>>
>> boolean fulltest;
>> if (validBits == cm.validBits) {
>> fulltest = false;
>> } else if (validBits == null || cm.validBits == null) {
>> return false;
>> } else if (validBits.equals(cm.validBits)) {
>> fulltest = false;
>> } else {
>> fulltest = true;
>> }
>>
>> if (fulltest) {
>> compare both rgb[] and validBits() values } else {
>> compare rgb[] entries alone
>> }
>>
>> In this case we are using validBits.equals() to reduce the amount of
>> testing in the loop, but are not basing a conclusion on whether the
>> result of the comparison will be true or false. equals() implies we
>> don't need to compare its values bit by bit, but !equals() doesn't
>> mean that the ICMs aren't equals()...
>>
>> ...jim
>
Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905: equals() method in IndexColorModel doesnt exist and it relies on ColorModel.equals() which is not strict
+1
-phil.
On 04/12/2016 01:19 PM, Jim Graham wrote:
On 4/12/2016 12:59 PM, Phil Race wrote:
I am catching up on email here, and "+1" but a couple of comments
- I suppose that we can't learn anything useful from
"cm.validbits.equals(this.validbits)"
since only the bits up to "map_size" should be tested ?
Perhaps if the constructors truncated it at the required size we could
use equals(). I'm not sure that is worthwhile, given how rarely it is
used. I think it is used by default on some platforms (Windows?
X11?) if they have an 8-bit screen with a sparse colormap, but that
should be pretty rare these days since nearly everything we use should
be 24-bits these days.
I have a better idea, though.
But, since the field is assigned the original supplied value from the
constructor, then the likelihood that a non-null value will be ==
identical to another colormap is likely larger than normal, perhaps we
can make it faster by checking for == and then slipping into the
faster test that simply compares the rgb[] values? Something like:
if (validBits == cm.validBits) {
compare rgb[] entries
} else if (validBits == null || cm.validBits == null) {
return false;
} else {
bigger loop that compares rgb[] and validBits() values
}
Note that if the two fields are == and non-null, then the entries in
the rgb[] array for indices that are non-valid should have zeros in
them because of the way that the colormap data is copied internally,
so the rgb[] comparisons should be valid without checking the
associated bits.
Potentially we could also use:
boolean fulltest;
if (validBits == cm.validBits) {
fulltest = false;
} else if (validBits == null || cm.validBits == null) {
return false;
} else if (validBits.equals(cm.validBits)) {
fulltest = false;
} else {
fulltest = true;
}
if (fulltest) {
compare both rgb[] and validBits() values
} else {
compare rgb[] entries alone
}
In this case we are using validBits.equals() to reduce the amount of
testing in the loop, but are not basing a conclusion on whether the
result of the comparison will be true or false. equals() implies we
don't need to compare its values bit by bit, but !equals() doesn't
mean that the ICMs aren't equals()...
...jim
Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905: equals() method in IndexColorModel doesnt exist and it relies on ColorModel.equals() which is not strict
Hi,
Thanks Phil & Jim for your suggestion.
I have made changes mentioned by Jim for whether we have validate individual
bits of validBits or not in different conditions.
Please find updated webrev for review :
http://cr.openjdk.java.net/~jdv/7107905/webrev.04/
Thanks,
Jay
-Original Message-
From: Phil Race
Sent: Wednesday, April 13, 2016 1:49 AM
To: Jim Graham
Cc: Jayathirth D V; Prasanta Sadhukhan; 2d-dev@openjdk.java.net
Subject: Re: Review Request for JDK-7107905: equals() method in IndexColorModel
doesnt exist and it relies on ColorModel.equals() which is not strict
Seems like this would work/help.
-phil.
On 04/12/2016 01:19 PM, Jim Graham wrote:
>
>
> On 4/12/2016 12:59 PM, Phil Race wrote:
>> I am catching up on email here, and "+1" but a couple of comments
>>
>> - I suppose that we can't learn anything useful from
>> "cm.validbits.equals(this.validbits)"
>> since only the bits up to "map_size" should be tested ?
>
> Perhaps if the constructors truncated it at the required size we could
> use equals(). I'm not sure that is worthwhile, given how rarely it is
> used. I think it is used by default on some platforms (Windows?
> X11?) if they have an 8-bit screen with a sparse colormap, but that
> should be pretty rare these days since nearly everything we use should
> be 24-bits these days.
>
> I have a better idea, though.
>
> But, since the field is assigned the original supplied value from the
> constructor, then the likelihood that a non-null value will be ==
> identical to another colormap is likely larger than normal, perhaps we
> can make it faster by checking for == and then slipping into the
> faster test that simply compares the rgb[] values? Something like:
>
> if (validBits == cm.validBits) {
> compare rgb[] entries
> } else if (validBits == null || cm.validBits == null) {
> return false;
> } else {
> bigger loop that compares rgb[] and validBits() values }
>
> Note that if the two fields are == and non-null, then the entries in
> the rgb[] array for indices that are non-valid should have zeros in
> them because of the way that the colormap data is copied internally,
> so the rgb[] comparisons should be valid without checking the
> associated bits.
>
> Potentially we could also use:
>
> boolean fulltest;
> if (validBits == cm.validBits) {
> fulltest = false;
> } else if (validBits == null || cm.validBits == null) {
> return false;
> } else if (validBits.equals(cm.validBits)) {
> fulltest = false;
> } else {
> fulltest = true;
> }
>
> if (fulltest) {
> compare both rgb[] and validBits() values } else {
> compare rgb[] entries alone
> }
>
> In this case we are using validBits.equals() to reduce the amount of
> testing in the loop, but are not basing a conclusion on whether the
> result of the comparison will be true or false. equals() implies we
> don't need to compare its values bit by bit, but !equals() doesn't
> mean that the ICMs aren't equals()...
>
> ...jim
Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905: equals() method in IndexColorModel doesnt exist and it relies on ColorModel.equals() which is not strict
Seems like this would work/help.
-phil.
On 04/12/2016 01:19 PM, Jim Graham wrote:
On 4/12/2016 12:59 PM, Phil Race wrote:
I am catching up on email here, and "+1" but a couple of comments
- I suppose that we can't learn anything useful from
"cm.validbits.equals(this.validbits)"
since only the bits up to "map_size" should be tested ?
Perhaps if the constructors truncated it at the required size we could
use equals(). I'm not sure that is worthwhile, given how rarely it is
used. I think it is used by default on some platforms (Windows?
X11?) if they have an 8-bit screen with a sparse colormap, but that
should be pretty rare these days since nearly everything we use should
be 24-bits these days.
I have a better idea, though.
But, since the field is assigned the original supplied value from the
constructor, then the likelihood that a non-null value will be ==
identical to another colormap is likely larger than normal, perhaps we
can make it faster by checking for == and then slipping into the
faster test that simply compares the rgb[] values? Something like:
if (validBits == cm.validBits) {
compare rgb[] entries
} else if (validBits == null || cm.validBits == null) {
return false;
} else {
bigger loop that compares rgb[] and validBits() values
}
Note that if the two fields are == and non-null, then the entries in
the rgb[] array for indices that are non-valid should have zeros in
them because of the way that the colormap data is copied internally,
so the rgb[] comparisons should be valid without checking the
associated bits.
Potentially we could also use:
boolean fulltest;
if (validBits == cm.validBits) {
fulltest = false;
} else if (validBits == null || cm.validBits == null) {
return false;
} else if (validBits.equals(cm.validBits)) {
fulltest = false;
} else {
fulltest = true;
}
if (fulltest) {
compare both rgb[] and validBits() values
} else {
compare rgb[] entries alone
}
In this case we are using validBits.equals() to reduce the amount of
testing in the loop, but are not basing a conclusion on whether the
result of the comparison will be true or false. equals() implies we
don't need to compare its values bit by bit, but !equals() doesn't
mean that the ICMs aren't equals()...
...jim
Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905: equals() method in IndexColorModel doesnt exist and it relies on ColorModel.equals() which is not strict
On 4/12/2016 12:59 PM, Phil Race wrote:
I am catching up on email here, and "+1" but a couple of comments
- I suppose that we can't learn anything useful from
"cm.validbits.equals(this.validbits)"
since only the bits up to "map_size" should be tested ?
Perhaps if the constructors truncated it at the required size we could
use equals(). I'm not sure that is worthwhile, given how rarely it is
used. I think it is used by default on some platforms (Windows? X11?)
if they have an 8-bit screen with a sparse colormap, but that should be
pretty rare these days since nearly everything we use should be 24-bits
these days.
I have a better idea, though.
But, since the field is assigned the original supplied value from the
constructor, then the likelihood that a non-null value will be ==
identical to another colormap is likely larger than normal, perhaps we
can make it faster by checking for == and then slipping into the faster
test that simply compares the rgb[] values? Something like:
if (validBits == cm.validBits) {
compare rgb[] entries
} else if (validBits == null || cm.validBits == null) {
return false;
} else {
bigger loop that compares rgb[] and validBits() values
}
Note that if the two fields are == and non-null, then the entries in the
rgb[] array for indices that are non-valid should have zeros in them
because of the way that the colormap data is copied internally, so the
rgb[] comparisons should be valid without checking the associated bits.
Potentially we could also use:
boolean fulltest;
if (validBits == cm.validBits) {
fulltest = false;
} else if (validBits == null || cm.validBits == null) {
return false;
} else if (validBits.equals(cm.validBits)) {
fulltest = false;
} else {
fulltest = true;
}
if (fulltest) {
compare both rgb[] and validBits() values
} else {
compare rgb[] entries alone
}
In this case we are using validBits.equals() to reduce the amount of
testing in the loop, but are not basing a conclusion on whether the
result of the comparison will be true or false. equals() implies we
don't need to compare its values bit by bit, but !equals() doesn't mean
that the ICMs aren't equals()...
...jim
Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905: equals() method in IndexColorModel doesnt exist and it relies on ColorModel.equals() which is not strict
I am catching up on email here, and "+1" but a couple of comments
- ColorModel.hashCode() doesn't say a lot about how it is calculated
so it seems safe to change it. Sometimes it makes performance sense
to cache the calculated value but in this case there is probably no
big win from doing so since it is not likely a commonly used case.
Also equals looks unavoidably expensive.
- I suppose that we can't learn anything useful from
"cm.validbits.equals(this.validbits)"
since only the bits up to "map_size" should be tested ?
- As Sergey noted, do file a CCC - this has been general practice when
over-riding equals or hashCode. I am not sure if you will get asked
there to say anything about what is meant by "equals()" or or how
"hashCode()" is calculated. You should get that CCC approved before
committing.
-phil.
On 04/12/2016 12:45 PM, Jim Graham wrote:
Hi Jay,
Looks great! Good to go...
...jim
On 4/12/2016 4:36 AM, Jayathirth D V wrote:
Hi Jim,
I have updated the webrev to include changes to check testBit()
instead of using isValid().
Please find the updated webrev for review:
http://cr.openjdk.java.net/~jdv/7107905/webrev.03/
Thanks,
Jay
-Original Message-
From: Jim Graham
Sent: Tuesday, April 12, 2016 12:21 AM
To: Jayathirth D V; Philip Race; Prasanta Sadhukhan
Cc: 2d-dev@openjdk.java.net
Subject: Re: Review Request for JDK-7107905: equals() method in
IndexColorModel doesnt exist and it relies on ColorModel.equals()
which is not strict
Hi Jay,
There was one thing I pointed out in the first review that got lost
in the shuffle. When the validBits are not null you use isValid(i)
to test the values, but that method does 3 things:
- tests the index for validity, but we already know it is valid
- tests validBits for null, but we already know it is not
- calls validBits.testBit() - this is the only part we need
For performance, I'd switch to just testing for the bits directly, as
in:
validBits.testBit(i) == cm.validBits.testBit(i)
...jim
On 4/11/2016 12:46 AM, Jayathirth D V wrote:
Hi Jim,
Thanks for the review.
I have made all recommended changes and created new subtask for
JDK-6588409(https://bugs.openjdk.java.net/browse/JDK-8153943 ).
Please review updated webrev:
http://cr.openjdk.java.net/~jdv/7107905/webrev.02/
Thanks,
Jay
-Original Message-
From: Jim Graham
Sent: Friday, April 08, 2016 12:28 AM
To: Jayathirth D V; Philip Race; Prasanta Sadhukhan
Cc: 2d-dev@openjdk.java.net
Subject: Re: Review Request for JDK-7107905: equals() method in
IndexColorModel doesnt exist and it relies on ColorModel.equals()
which is not strict
Hi Jayathirth,
This looks good.
One thing to note for efficiency, "instanceof" also checks for null.
It only returns true for non-null objects, so you don't need to
explicitly test for null at the top of ICM.equals(). (Though, you
should include a
test(s) in the unit test that verifies that no ICM returns true for
equals(null) to be sure.) You can see that CM.equals() is
implemented this way.
Also, for performance, ICM should include a quick "if (this == cm)
return true;" check, like CM.equals(). I'd recommend:
- first instanceof
- then == test
- then super.equals()
- finally, test equality of data fields
More comments inline:
On 4/7/16 6:46 AM, Jayathirth D V wrote:
- Yes https://bugs.openjdk.java.net/browse/JDK-6588409 has
mentioned the same thing. Can I create a subtask to address
java.awt.image changes?
That would be good.
For now, it would be good to implement hashCode() on ICM now that
you are creating an equals() method for it.
- I am not completely sure of what is the optimal way of adding
hashCode() API so I took help from internet and IDE to make the
changes. I am including super.hashCode() as it adds uniqueness to ICM.
You did a great job. To save time in the future, you should all
have copies of the latest version of "Effective Java" by Josh
Bloch. It has a whole chapter on equals/hashCode. It's a very
handy reference for all sorts of good programming practice for the
Java language.
Also, ColorModel.hashCode() is a poor implementation. It doesn't
use the paradigms recommended by Effective Java and looks like it
produces poor hashes as a result (just in the first two elements of
the hashCode I see a collision due to poor choice of numbers).
- I think since we are not using Prime numbers and it might
result in improper hash code. I have made similar changes to
hashCode() of ColorModel.
Looks great.
- In the same file itself we are following Java coding
guidelines of having braces after if like "if () {". I am not
completely sure of including "{" in new line.
You are correct, that matching local code is a good consideration
when considering code style. In this case, though, the indentation
on these if statements is an example of what we're trying to avoid
so it would be better to fix these particular statements (don't
bother
Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905: equals() method in IndexColorModel doesnt exist and it relies on ColorModel.equals() which is not strict
Hi Jay,
Looks great! Good to go...
...jim
On 4/12/2016 4:36 AM, Jayathirth D V wrote:
Hi Jim,
I have updated the webrev to include changes to check testBit() instead of
using isValid().
Please find the updated webrev for review:
http://cr.openjdk.java.net/~jdv/7107905/webrev.03/
Thanks,
Jay
-Original Message-
From: Jim Graham
Sent: Tuesday, April 12, 2016 12:21 AM
To: Jayathirth D V; Philip Race; Prasanta Sadhukhan
Cc: 2d-dev@openjdk.java.net
Subject: Re: Review Request for JDK-7107905: equals() method in IndexColorModel
doesnt exist and it relies on ColorModel.equals() which is not strict
Hi Jay,
There was one thing I pointed out in the first review that got lost in the
shuffle. When the validBits are not null you use isValid(i) to test the
values, but that method does 3 things:
- tests the index for validity, but we already know it is valid
- tests validBits for null, but we already know it is not
- calls validBits.testBit() - this is the only part we need
For performance, I'd switch to just testing for the bits directly, as in:
validBits.testBit(i) == cm.validBits.testBit(i)
...jim
On 4/11/2016 12:46 AM, Jayathirth D V wrote:
Hi Jim,
Thanks for the review.
I have made all recommended changes and created new subtask for
JDK-6588409(https://bugs.openjdk.java.net/browse/JDK-8153943 ).
Please review updated webrev:
http://cr.openjdk.java.net/~jdv/7107905/webrev.02/
Thanks,
Jay
-Original Message-
From: Jim Graham
Sent: Friday, April 08, 2016 12:28 AM
To: Jayathirth D V; Philip Race; Prasanta Sadhukhan
Cc: 2d-dev@openjdk.java.net
Subject: Re: Review Request for JDK-7107905: equals() method in
IndexColorModel doesnt exist and it relies on ColorModel.equals()
which is not strict
Hi Jayathirth,
This looks good.
One thing to note for efficiency, "instanceof" also checks for null.
It only returns true for non-null objects, so you don't need to
explicitly test for null at the top of ICM.equals(). (Though, you
should include a
test(s) in the unit test that verifies that no ICM returns true for
equals(null) to be sure.) You can see that CM.equals() is implemented this way.
Also, for performance, ICM should include a quick "if (this == cm) return
true;" check, like CM.equals(). I'd recommend:
- first instanceof
- then == test
- then super.equals()
- finally, test equality of data fields
More comments inline:
On 4/7/16 6:46 AM, Jayathirth D V wrote:
- Yes https://bugs.openjdk.java.net/browse/JDK-6588409 has mentioned
the same thing. Can I create a subtask to address java.awt.image changes?
That would be good.
For now, it would be good to implement hashCode() on ICM now that you are
creating an equals() method for it.
- I am not completely sure of what is the optimal way of adding
hashCode() API so I took help from internet and IDE to make the changes. I am
including super.hashCode() as it adds uniqueness to ICM.
You did a great job. To save time in the future, you should all have copies of the
latest version of "Effective Java" by Josh Bloch. It has a whole chapter on
equals/hashCode. It's a very handy reference for all sorts of good programming practice
for the Java language.
Also, ColorModel.hashCode() is a poor implementation. It doesn't use the
paradigms recommended by Effective Java and looks like it produces poor hashes
as a result (just in the first two elements of the hashCode I see a collision
due to poor choice of numbers).
- I think since we are not using Prime numbers and it might result in
improper hash code. I have made similar changes to hashCode() of ColorModel.
Looks great.
- In the same file itself we are following Java coding guidelines of having braces after if
like "if () {". I am not completely sure of including "{" in new line.
You are correct, that matching local code is a good consideration when
considering code style. In this case, though, the indentation on these if
statements is an example of what we're trying to avoid so it would be better to
fix these particular statements (don't bother fixing the other lines in the
file at this point, that can wait until we have to update other parts of the
file, but don't propagate a bad style in new code).
In particular:
Never do this:
if (some complex test ||
some additional tests ||
some additional tests ||
some additional tests ||
some continuation of the test) {
the body of the code;
more body of the code;
}
Quick question - where does the body of the if statement start? Does your eye
see it in a fraction of a second or do you have to search for it?
That is the worst option for indenting an if statement with continuation lines.
Never do that in new code. Do either of the following two versions:
Java Code Style guidelines recommends indenting 8 spaces for
conditional
continuations:
Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905: equals() method in IndexColorModel doesnt exist and it relies on ColorModel.equals() which is not strict
Hi Jim,
I have updated the webrev to include changes to check testBit() instead of
using isValid().
Please find the updated webrev for review:
http://cr.openjdk.java.net/~jdv/7107905/webrev.03/
Thanks,
Jay
-Original Message-
From: Jim Graham
Sent: Tuesday, April 12, 2016 12:21 AM
To: Jayathirth D V; Philip Race; Prasanta Sadhukhan
Cc: 2d-dev@openjdk.java.net
Subject: Re: Review Request for JDK-7107905: equals() method in IndexColorModel
doesnt exist and it relies on ColorModel.equals() which is not strict
Hi Jay,
There was one thing I pointed out in the first review that got lost in the
shuffle. When the validBits are not null you use isValid(i) to test the
values, but that method does 3 things:
- tests the index for validity, but we already know it is valid
- tests validBits for null, but we already know it is not
- calls validBits.testBit() - this is the only part we need
For performance, I'd switch to just testing for the bits directly, as in:
validBits.testBit(i) == cm.validBits.testBit(i)
...jim
On 4/11/2016 12:46 AM, Jayathirth D V wrote:
> Hi Jim,
>
> Thanks for the review.
> I have made all recommended changes and created new subtask for
> JDK-6588409(https://bugs.openjdk.java.net/browse/JDK-8153943 ).
>
> Please review updated webrev:
> http://cr.openjdk.java.net/~jdv/7107905/webrev.02/
>
> Thanks,
> Jay
>
> -Original Message-
> From: Jim Graham
> Sent: Friday, April 08, 2016 12:28 AM
> To: Jayathirth D V; Philip Race; Prasanta Sadhukhan
> Cc: 2d-dev@openjdk.java.net
> Subject: Re: Review Request for JDK-7107905: equals() method in
> IndexColorModel doesnt exist and it relies on ColorModel.equals()
> which is not strict
>
> Hi Jayathirth,
>
> This looks good.
>
> One thing to note for efficiency, "instanceof" also checks for null.
> It only returns true for non-null objects, so you don't need to
> explicitly test for null at the top of ICM.equals(). (Though, you
> should include a
> test(s) in the unit test that verifies that no ICM returns true for
> equals(null) to be sure.) You can see that CM.equals() is implemented this
> way.
>
> Also, for performance, ICM should include a quick "if (this == cm) return
> true;" check, like CM.equals(). I'd recommend:
>
> - first instanceof
> - then == test
> - then super.equals()
> - finally, test equality of data fields
>
> More comments inline:
>
> On 4/7/16 6:46 AM, Jayathirth D V wrote:
>> - Yes https://bugs.openjdk.java.net/browse/JDK-6588409 has mentioned
>> the same thing. Can I create a subtask to address java.awt.image changes?
>
> That would be good.
>
>> For now, it would be good to implement hashCode() on ICM now that you are
>> creating an equals() method for it.
>>
>> - I am not completely sure of what is the optimal way of adding
>> hashCode() API so I took help from internet and IDE to make the changes. I
>> am including super.hashCode() as it adds uniqueness to ICM.
>
> You did a great job. To save time in the future, you should all have copies
> of the latest version of "Effective Java" by Josh Bloch. It has a whole
> chapter on equals/hashCode. It's a very handy reference for all sorts of
> good programming practice for the Java language.
>
>> Also, ColorModel.hashCode() is a poor implementation. It doesn't use the
>> paradigms recommended by Effective Java and looks like it produces poor
>> hashes as a result (just in the first two elements of the hashCode I see a
>> collision due to poor choice of numbers).
>> - I think since we are not using Prime numbers and it might result in
>> improper hash code. I have made similar changes to hashCode() of ColorModel.
>
> Looks great.
>
>> - In the same file itself we are following Java coding guidelines of
>> having braces after if like "if () {". I am not completely sure of including
>> "{" in new line.
>
> You are correct, that matching local code is a good consideration when
> considering code style. In this case, though, the indentation on these if
> statements is an example of what we're trying to avoid so it would be better
> to fix these particular statements (don't bother fixing the other lines in
> the file at this point, that can wait until we have to update other parts of
> the file, but don't propagate a bad style in new code).
> In particular:
>
> Never do this:
>
> if (some complex test ||
> some additional tests ||
> some additional tests ||
> some additional tests ||
> some continuation of the test) {
> the body of the code;
> more body of the code;
> }
> Quick question - where does the body of the if statement start? Does your
> eye see it in a fraction of a second or do you have to search for it?
>
> That is the worst option for indenting an if statement with continuation
> lines. Never do that in new code. Do either of the following two versions:
>
> Java Code Style guidelines recommends
Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905: equals() method in IndexColorModel doesnt exist and it relies on ColorModel.equals() which is not strict
Hi Jay,
There was one thing I pointed out in the first review that got lost in
the shuffle. When the validBits are not null you use isValid(i) to test
the values, but that method does 3 things:
- tests the index for validity, but we already know it is valid
- tests validBits for null, but we already know it is not
- calls validBits.testBit() - this is the only part we need
For performance, I'd switch to just testing for the bits directly, as in:
validBits.testBit(i) == cm.validBits.testBit(i)
...jim
On 4/11/2016 12:46 AM, Jayathirth D V wrote:
Hi Jim,
Thanks for the review.
I have made all recommended changes and created new subtask for
JDK-6588409(https://bugs.openjdk.java.net/browse/JDK-8153943 ).
Please review updated webrev:
http://cr.openjdk.java.net/~jdv/7107905/webrev.02/
Thanks,
Jay
-Original Message-
From: Jim Graham
Sent: Friday, April 08, 2016 12:28 AM
To: Jayathirth D V; Philip Race; Prasanta Sadhukhan
Cc: 2d-dev@openjdk.java.net
Subject: Re: Review Request for JDK-7107905: equals() method in IndexColorModel
doesnt exist and it relies on ColorModel.equals() which is not strict
Hi Jayathirth,
This looks good.
One thing to note for efficiency, "instanceof" also checks for null. It only
returns true for non-null objects, so you don't need to explicitly test for null at the
top of ICM.equals(). (Though, you should include a
test(s) in the unit test that verifies that no ICM returns true for
equals(null) to be sure.) You can see that CM.equals() is implemented this way.
Also, for performance, ICM should include a quick "if (this == cm) return
true;" check, like CM.equals(). I'd recommend:
- first instanceof
- then == test
- then super.equals()
- finally, test equality of data fields
More comments inline:
On 4/7/16 6:46 AM, Jayathirth D V wrote:
- Yes https://bugs.openjdk.java.net/browse/JDK-6588409 has mentioned
the same thing. Can I create a subtask to address java.awt.image changes?
That would be good.
For now, it would be good to implement hashCode() on ICM now that you are
creating an equals() method for it.
- I am not completely sure of what is the optimal way of adding
hashCode() API so I took help from internet and IDE to make the changes. I am
including super.hashCode() as it adds uniqueness to ICM.
You did a great job. To save time in the future, you should all have copies of the
latest version of "Effective Java" by Josh Bloch. It has a whole chapter on
equals/hashCode. It's a very handy reference for all sorts of good programming practice
for the Java language.
Also, ColorModel.hashCode() is a poor implementation. It doesn't use the
paradigms recommended by Effective Java and looks like it produces poor hashes
as a result (just in the first two elements of the hashCode I see a collision
due to poor choice of numbers).
- I think since we are not using Prime numbers and it might result in
improper hash code. I have made similar changes to hashCode() of ColorModel.
Looks great.
- In the same file itself we are following Java coding guidelines of having braces after if
like "if () {". I am not completely sure of including "{" in new line.
You are correct, that matching local code is a good consideration when
considering code style. In this case, though, the indentation on these if
statements is an example of what we're trying to avoid so it would be better to
fix these particular statements (don't bother fixing the other lines in the
file at this point, that can wait until we have to update other parts of the
file, but don't propagate a bad style in new code).
In particular:
Never do this:
if (some complex test ||
some additional tests ||
some additional tests ||
some additional tests ||
some continuation of the test) {
the body of the code;
more body of the code;
}
Quick question - where does the body of the if statement start? Does your eye
see it in a fraction of a second or do you have to search for it?
That is the worst option for indenting an if statement with continuation lines.
Never do that in new code. Do either of the following two versions:
Java Code Style guidelines recommends indenting 8 spaces for conditional
continuations:
if (some complex test ||
some additional tests ||
some additional tests ||
some additional tests ||
some continuation of the test) {
the body of the code;
more body of the code;
}
Jim's personal extension to the JCS recommends (and the 2D team historically
tended to agree):
if (some complex test ||
some additional tests ||
some additional tests ||
some additional tests ||
some continuation of the test)
{
the body of the code;
more body of the code;
}
Both of those immediately
Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905: equals() method in IndexColorModel doesnt exist and it relies on ColorModel.equals() which is not strict
Sure Sergey after technical review is done I will raise CCC for the same.
Thanks,
Jay
-Original Message-
From: Sergey Bylokhov
Sent: Friday, April 08, 2016 12:42 AM
To: Jim Graham; Jayathirth D V; Philip Race; Prasanta Sadhukhan
Cc: 2d-dev@openjdk.java.net
Subject: Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905: equals() method
in IndexColorModel doesnt exist and it relies on ColorModel.equals() which is
not strict
Small note that we should not forget to create a ccc.
On 07.04.16 21:58, Jim Graham wrote:
> Hi Jayathirth,
>
> This looks good.
>
> One thing to note for efficiency, "instanceof" also checks for null.
> It only returns true for non-null objects, so you don't need to
> explicitly test for null at the top of ICM.equals(). (Though, you
> should include a
> test(s) in the unit test that verifies that no ICM returns true for
> equals(null) to be sure.) You can see that CM.equals() is implemented
> this way.
>
> Also, for performance, ICM should include a quick "if (this == cm)
> return true;" check, like CM.equals(). I'd recommend:
>
> - first instanceof
> - then == test
> - then super.equals()
> - finally, test equality of data fields
>
> More comments inline:
>
> On 4/7/16 6:46 AM, Jayathirth D V wrote:
>> - Yes https://bugs.openjdk.java.net/browse/JDK-6588409 has
>> mentioned the same thing. Can I create a subtask to address
>> java.awt.image changes?
>
> That would be good.
>
>> For now, it would be good to implement hashCode() on ICM now that you
>> are creating an equals() method for it.
>>
>> - I am not completely sure of what is the optimal way of adding
>> hashCode() API so I took help from internet and IDE to make the
>> changes. I am including super.hashCode() as it adds uniqueness to ICM.
>
> You did a great job. To save time in the future, you should all have
> copies of the latest version of "Effective Java" by Josh Bloch. It
> has a whole chapter on equals/hashCode. It's a very handy reference
> for all sorts of good programming practice for the Java language.
>
>> Also, ColorModel.hashCode() is a poor implementation. It doesn't use
>> the paradigms recommended by Effective Java and looks like it
>> produces poor hashes as a result (just in the first two elements of
>> the hashCode I see a collision due to poor choice of numbers).
>> - I think since we are not using Prime numbers and it might
>> result in improper hash code. I have made similar changes to
>> hashCode() of ColorModel.
>
> Looks great.
>
>> - In the same file itself we are following Java coding guidelines
>> of having braces after if like "if () {". I am not completely sure of
>> including "{" in new line.
>
> You are correct, that matching local code is a good consideration when
> considering code style. In this case, though, the indentation on
> these if statements is an example of what we're trying to avoid so it
> would be better to fix these particular statements (don't bother
> fixing the other lines in the file at this point, that can wait until
> we have to update other parts of the file, but don't propagate a bad style in
> new code).
> In particular:
>
> Never do this:
>
> if (some complex test ||
> some additional tests ||
> some additional tests ||
> some additional tests ||
> some continuation of the test) {
> the body of the code;
> more body of the code;
> }
> Quick question - where does the body of the if statement start? Does
> your eye see it in a fraction of a second or do you have to search for it?
>
> That is the worst option for indenting an if statement with
> continuation lines. Never do that in new code. Do either of the
> following two
> versions:
>
> Java Code Style guidelines recommends indenting 8 spaces for
> conditional
> continuations:
> if (some complex test ||
> some additional tests ||
> some additional tests ||
> some additional tests ||
> some continuation of the test) {
> the body of the code;
> more body of the code;
> }
>
> Jim's personal extension to the JCS recommends (and the 2D team
> historically tended to agree):
> if (some complex test ||
> some additional tests ||
> some additional tests ||
> some additional tests ||
> some continuation of the test)
> {
> the body of the code;
> more body of the code;
> }
>
> Both of those immediately draw the eye to the separating point between
> the conditional and the body of the code.
>
>> I'd also add a few test cases that test that 2 separately and
>> identically constructed ICM instances are equals() == true, tested
>> with one of each of the constructors...
>>
>> - I have made changes to test case for verifying all constructors
>> with same ICM. Also added verification for hashCode value.
>
> Unfortunately, some of your tests for
Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905: equals() method in IndexColorModel doesnt exist and it relies on ColorModel.equals() which is not strict
Hi Jim,
Thanks for the review.
I have made all recommended changes and created new subtask for
JDK-6588409(https://bugs.openjdk.java.net/browse/JDK-8153943 ).
Please review updated webrev:
http://cr.openjdk.java.net/~jdv/7107905/webrev.02/
Thanks,
Jay
-Original Message-
From: Jim Graham
Sent: Friday, April 08, 2016 12:28 AM
To: Jayathirth D V; Philip Race; Prasanta Sadhukhan
Cc: 2d-dev@openjdk.java.net
Subject: Re: Review Request for JDK-7107905: equals() method in IndexColorModel
doesnt exist and it relies on ColorModel.equals() which is not strict
Hi Jayathirth,
This looks good.
One thing to note for efficiency, "instanceof" also checks for null. It only
returns true for non-null objects, so you don't need to explicitly test for
null at the top of ICM.equals(). (Though, you should include a
test(s) in the unit test that verifies that no ICM returns true for
equals(null) to be sure.) You can see that CM.equals() is implemented this way.
Also, for performance, ICM should include a quick "if (this == cm) return
true;" check, like CM.equals(). I'd recommend:
- first instanceof
- then == test
- then super.equals()
- finally, test equality of data fields
More comments inline:
On 4/7/16 6:46 AM, Jayathirth D V wrote:
> - Yes https://bugs.openjdk.java.net/browse/JDK-6588409 has mentioned
> the same thing. Can I create a subtask to address java.awt.image changes?
That would be good.
> For now, it would be good to implement hashCode() on ICM now that you are
> creating an equals() method for it.
>
> - I am not completely sure of what is the optimal way of adding
> hashCode() API so I took help from internet and IDE to make the changes. I am
> including super.hashCode() as it adds uniqueness to ICM.
You did a great job. To save time in the future, you should all have copies of
the latest version of "Effective Java" by Josh Bloch. It has a whole chapter
on equals/hashCode. It's a very handy reference for all sorts of good
programming practice for the Java language.
> Also, ColorModel.hashCode() is a poor implementation. It doesn't use the
> paradigms recommended by Effective Java and looks like it produces poor
> hashes as a result (just in the first two elements of the hashCode I see a
> collision due to poor choice of numbers).
> - I think since we are not using Prime numbers and it might result in
> improper hash code. I have made similar changes to hashCode() of ColorModel.
Looks great.
> - In the same file itself we are following Java coding guidelines of
> having braces after if like "if () {". I am not completely sure of including
> "{" in new line.
You are correct, that matching local code is a good consideration when
considering code style. In this case, though, the indentation on these if
statements is an example of what we're trying to avoid so it would be better to
fix these particular statements (don't bother fixing the other lines in the
file at this point, that can wait until we have to update other parts of the
file, but don't propagate a bad style in new code).
In particular:
Never do this:
if (some complex test ||
some additional tests ||
some additional tests ||
some additional tests ||
some continuation of the test) {
the body of the code;
more body of the code;
}
Quick question - where does the body of the if statement start? Does your eye
see it in a fraction of a second or do you have to search for it?
That is the worst option for indenting an if statement with continuation lines.
Never do that in new code. Do either of the following two versions:
Java Code Style guidelines recommends indenting 8 spaces for conditional
continuations:
if (some complex test ||
some additional tests ||
some additional tests ||
some additional tests ||
some continuation of the test) {
the body of the code;
more body of the code;
}
Jim's personal extension to the JCS recommends (and the 2D team historically
tended to agree):
if (some complex test ||
some additional tests ||
some additional tests ||
some additional tests ||
some continuation of the test)
{
the body of the code;
more body of the code;
}
Both of those immediately draw the eye to the separating point between the
conditional and the body of the code.
> I'd also add a few test cases that test that 2 separately and identically
> constructed ICM instances are equals() == true, tested with one of each of
> the constructors...
>
> - I have made changes to test case for verifying all constructors with
> same ICM. Also added verification for hashCode value.
Unfortunately, some of your tests for hashCode make an invalid assumption. It
is technically correct for the hash codes of 2 different objects to be the same
regardless of whether they
Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905: equals() method in IndexColorModel doesnt exist and it relies on ColorModel.equals() which is not strict
Small note that we should not forget to create a ccc.
On 07.04.16 21:58, Jim Graham wrote:
Hi Jayathirth,
This looks good.
One thing to note for efficiency, "instanceof" also checks for null. It
only returns true for non-null objects, so you don't need to explicitly
test for null at the top of ICM.equals(). (Though, you should include a
test(s) in the unit test that verifies that no ICM returns true for
equals(null) to be sure.) You can see that CM.equals() is implemented
this way.
Also, for performance, ICM should include a quick "if (this == cm)
return true;" check, like CM.equals(). I'd recommend:
- first instanceof
- then == test
- then super.equals()
- finally, test equality of data fields
More comments inline:
On 4/7/16 6:46 AM, Jayathirth D V wrote:
- Yes https://bugs.openjdk.java.net/browse/JDK-6588409 has
mentioned the same thing. Can I create a subtask to address
java.awt.image changes?
That would be good.
For now, it would be good to implement hashCode() on ICM now that you
are creating an equals() method for it.
- I am not completely sure of what is the optimal way of adding
hashCode() API so I took help from internet and IDE to make the
changes. I am including super.hashCode() as it adds uniqueness to ICM.
You did a great job. To save time in the future, you should all have
copies of the latest version of "Effective Java" by Josh Bloch. It has
a whole chapter on equals/hashCode. It's a very handy reference for all
sorts of good programming practice for the Java language.
Also, ColorModel.hashCode() is a poor implementation. It doesn't use
the paradigms recommended by Effective Java and looks like it produces
poor hashes as a result (just in the first two elements of the
hashCode I see a collision due to poor choice of numbers).
- I think since we are not using Prime numbers and it might result
in improper hash code. I have made similar changes to hashCode() of
ColorModel.
Looks great.
- In the same file itself we are following Java coding guidelines
of having braces after if like "if () {". I am not completely sure of
including "{" in new line.
You are correct, that matching local code is a good consideration when
considering code style. In this case, though, the indentation on these
if statements is an example of what we're trying to avoid so it would be
better to fix these particular statements (don't bother fixing the other
lines in the file at this point, that can wait until we have to update
other parts of the file, but don't propagate a bad style in new code).
In particular:
Never do this:
if (some complex test ||
some additional tests ||
some additional tests ||
some additional tests ||
some continuation of the test) {
the body of the code;
more body of the code;
}
Quick question - where does the body of the if statement start? Does
your eye see it in a fraction of a second or do you have to search for it?
That is the worst option for indenting an if statement with continuation
lines. Never do that in new code. Do either of the following two
versions:
Java Code Style guidelines recommends indenting 8 spaces for conditional
continuations:
if (some complex test ||
some additional tests ||
some additional tests ||
some additional tests ||
some continuation of the test) {
the body of the code;
more body of the code;
}
Jim's personal extension to the JCS recommends (and the 2D team
historically tended to agree):
if (some complex test ||
some additional tests ||
some additional tests ||
some additional tests ||
some continuation of the test)
{
the body of the code;
more body of the code;
}
Both of those immediately draw the eye to the separating point between
the conditional and the body of the code.
I'd also add a few test cases that test that 2 separately and
identically constructed ICM instances are equals() == true, tested
with one of each of the constructors...
- I have made changes to test case for verifying all constructors
with same ICM. Also added verification for hashCode value.
Unfortunately, some of your tests for hashCode make an invalid
assumption. It is technically correct for the hash codes of 2 different
objects to be the same regardless of whether they are equals() or not.
In fact, a perfectly valid implementation of hashCode() could return a
constant number and it would be correct from the perspective of the
equals/hashCode contract. (Such code, however, would not be optimal,
but it would be valid/correct-to-spec.) The only condition that is
required is that the hash codes match if the objects are equals(), but
they are allowed to match if the objects are !equals(). In other words:
boolean equals1 = (o1.equals(o2));
boolean equals2 = (o2.equals(o1));
boolean equalsH =
Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905: equals() method in IndexColorModel doesnt exist and it relies on ColorModel.equals() which is not strict
Hi Jayathirth,
This looks good.
One thing to note for efficiency, "instanceof" also checks for null. It
only returns true for non-null objects, so you don't need to explicitly
test for null at the top of ICM.equals(). (Though, you should include a
test(s) in the unit test that verifies that no ICM returns true for
equals(null) to be sure.) You can see that CM.equals() is implemented
this way.
Also, for performance, ICM should include a quick "if (this == cm)
return true;" check, like CM.equals(). I'd recommend:
- first instanceof
- then == test
- then super.equals()
- finally, test equality of data fields
More comments inline:
On 4/7/16 6:46 AM, Jayathirth D V wrote:
- Yes https://bugs.openjdk.java.net/browse/JDK-6588409 has mentioned
the same thing. Can I create a subtask to address java.awt.image changes?
That would be good.
For now, it would be good to implement hashCode() on ICM now that you are
creating an equals() method for it.
- I am not completely sure of what is the optimal way of adding
hashCode() API so I took help from internet and IDE to make the changes. I am
including super.hashCode() as it adds uniqueness to ICM.
You did a great job. To save time in the future, you should all have
copies of the latest version of "Effective Java" by Josh Bloch. It has
a whole chapter on equals/hashCode. It's a very handy reference for all
sorts of good programming practice for the Java language.
Also, ColorModel.hashCode() is a poor implementation. It doesn't use the
paradigms recommended by Effective Java and looks like it produces poor hashes
as a result (just in the first two elements of the hashCode I see a collision
due to poor choice of numbers).
- I think since we are not using Prime numbers and it might result in
improper hash code. I have made similar changes to hashCode() of ColorModel.
Looks great.
- In the same file itself we are following Java coding guidelines of having braces after if
like "if () {". I am not completely sure of including "{" in new line.
You are correct, that matching local code is a good consideration when
considering code style. In this case, though, the indentation on these
if statements is an example of what we're trying to avoid so it would be
better to fix these particular statements (don't bother fixing the other
lines in the file at this point, that can wait until we have to update
other parts of the file, but don't propagate a bad style in new code).
In particular:
Never do this:
if (some complex test ||
some additional tests ||
some additional tests ||
some additional tests ||
some continuation of the test) {
the body of the code;
more body of the code;
}
Quick question - where does the body of the if statement start? Does
your eye see it in a fraction of a second or do you have to search for it?
That is the worst option for indenting an if statement with continuation
lines. Never do that in new code. Do either of the following two versions:
Java Code Style guidelines recommends indenting 8 spaces for conditional
continuations:
if (some complex test ||
some additional tests ||
some additional tests ||
some additional tests ||
some continuation of the test) {
the body of the code;
more body of the code;
}
Jim's personal extension to the JCS recommends (and the 2D team
historically tended to agree):
if (some complex test ||
some additional tests ||
some additional tests ||
some additional tests ||
some continuation of the test)
{
the body of the code;
more body of the code;
}
Both of those immediately draw the eye to the separating point between
the conditional and the body of the code.
I'd also add a few test cases that test that 2 separately and identically
constructed ICM instances are equals() == true, tested with one of each of the
constructors...
- I have made changes to test case for verifying all constructors with
same ICM. Also added verification for hashCode value.
Unfortunately, some of your tests for hashCode make an invalid
assumption. It is technically correct for the hash codes of 2 different
objects to be the same regardless of whether they are equals() or not.
In fact, a perfectly valid implementation of hashCode() could return a
constant number and it would be correct from the perspective of the
equals/hashCode contract. (Such code, however, would not be optimal,
but it would be valid/correct-to-spec.) The only condition that is
required is that the hash codes match if the objects are equals(), but
they are allowed to match if the objects are !equals(). In other words:
boolean equals1 = (o1.equals(o2));
boolean equals2 = (o2.equals(o1));
boolean equalsH = (o1.hashCode() == o2.hashCode());
if (equals1
Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905: equals() method in IndexColorModel doesnt exist and it relies on ColorModel.equals() which is not strict
Hi Jim,
Thanks for your valuable inputs.
Please find the updates inline. I have created new webrev
http://cr.openjdk.java.net/~jdv/7107905/webrev.01/ for review.
Are there any bugs in the database about the fact that many of these ColorModel
subclasses implement equals without hashcode? They really should both be
implemented, but since ColorModel implements the method based on its tests,
they are technically covered by the equals/hashCode contract - it's just not a
very optimal implementation of the contract.
- Yes https://bugs.openjdk.java.net/browse/JDK-6588409 has mentioned
the same thing. Can I create a subtask to address java.awt.image changes?
For now, it would be good to implement hashCode() on ICM now that you are
creating an equals() method for it.
- I am not completely sure of what is the optimal way of adding
hashCode() API so I took help from internet and IDE to make the changes. I am
including super.hashCode() as it adds uniqueness to ICM.
Also, ColorModel.hashCode() is a poor implementation. It doesn't use the
paradigms recommended by Effective Java and looks like it produces poor hashes
as a result (just in the first two elements of the hashCode I see a collision
due to poor choice of numbers).
- I think since we are not using Prime numbers and it might result in
improper hash code. I have made similar changes to hashCode() of ColorModel.
With respect to this particular equals() override I think it looks fine, though
the use of the isValid() method reduces its performance for a couple of reasons:
- it retests the index for the range [0,map size] which we already know is valid
- if the validBits is null, there is no need to even do the test.
This might be faster for the very common case:
if (validBits == null) {
if (cm.validBits != null) return false;
// loop just comparing rgb[] values
} else {
if (cm.validBits == null) return false;
// loop, testing rgb[] values and
// corresponding validBits indices directly
}
Note that in the constructor we set validBits to null if it is "true"
for all valid indices so if one of the cmaps has it null and the other does
not, then that is a very good indicator that their valid indices don't match.
- I have updated the suggested changes.
On a more minor note, I don't like the indentation of the if statement, I'd
move the "{" brace to a line of its own indented the same as its closing "}" to
make the body of the if stand out. It isn't strictly the Java coding
guidelines, but it does match a bunch of the other 2D code - sort of a local
exception to the coding style.
- In the same file itself we are following Java coding guidelines of
having braces after if like "if () {". I am not completely sure of including
"{" in new line.
I'd also add a few test cases that test that 2 separately and identically
constructed ICM instances are equals() == true, tested with one of each of the
constructors...
- I have made changes to test case for verifying all constructors with
same ICM. Also added verification for hashCode value.
...jim
On 4/6/2016 4:47 AM, Jayathirth D V wrote:
> Hi,
>
> _Please review the following fix in JDK9:_
>
> Bug : https://bugs.openjdk.java.net/browse/JDK-7107905
>
> Webrev : http://cr.openjdk.java.net/~jdv/7107905/webrev.00/
>
> Issue : When we compare two IndexColorModel objects with different
> ColorMap(or any other property specific to IndexColorModel) using
> equals() method it returns true.
>
> Root cause : There is no override equals() method specific to
> IndexColorModel and it uses super class equals() which is ColorModel
> and it doesn't verify equality for IndexColorModel specific properties.
>
> Solution : Add override equals() method for IndexColorModel which
> verifies its unique properties for equality.
>
> Thanks,
>
> Jay
>
