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

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