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