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

[Jim Graham]

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