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

[Phil Race]

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