+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 == 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
