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
