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
