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