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

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
>