[OpenJDK 2D-Dev] Review Request for JDK-7107905: equals() method in IndexColorModel doesnt exist and it relies on ColorModel.equals() which is not strict

[Jayathirth D V]

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