[OpenJDK 2D-Dev] Review Request for JDK-8153943 : In java.awt.image package some of the classes are missing hashCode() or equals() method

Jayathirth D V jayathirth.d.v at oracle.com
Thu May 5 07:11:01 UTC 2016


Hi Jim,

For ComponentColorModel will it be fine if we verify equals() and hashCode() based on following properties set in Constructor : transferType, signed, needScaleInit, noUnnorm, nonStdScale?

And for PixelInterleavedSampleModel and BandedSampleModel there are no unique properties set in constructor and they are calling super() directly and also I think hashCode() in these subclasses are not needed.
To differentiate discussion between ColorModel and SampleModel, I have started separate thread for questions related to PixelInterleavedSampleModel and BandedSampleModel with subject "equals() and hashCode() verification in java.awt.image.ComponentSampleModel and its subclassses".

Please provide your inputs.

Thanks,
Jay

-----Original Message-----
From: Jim Graham 
Sent: Wednesday, May 04, 2016 2:44 AM
To: Phil Race
Cc: 2d-dev at openjdk.java.net
Subject: Re: [OpenJDK 2D-Dev] Review Request for JDK-8153943 : In java.awt.image package some of the classes are missing hashCode() or equals() method

Yes, the equals/hashcode chapter in Effective Java includes rules about ignoring fields that can be calculated from other fields (which applies to most internal fields).  Basically, only things in the constructors tend to be good candidates for equals/hashcode...

			...jim

On 5/3/2016 2:00 PM, Phil Race wrote:
> On 04/26/2016 04:10 PM, Jim Graham wrote:
>> The ComponentColorModel implementation is a meaningless wrapper 
>> around super.equals/hashCode().  Why does it not test CCM-specific fields?
>
> It should although this looks like it is more than just running 
> through all the fields and testing them for equality.
> eg it looks like "initScale()" should be called if necessary before 
> determining equality and the field "needScaleInit" is not one that has 
> to be directly included in the equality comparison.
>
> -phil.
>
>
>
>>
>> The ComponentSampleModel.hashCode() method should be upgraded based 
>> on the recommendations in Effective Java like the other methods here.
>>
>> PixelInterleavedSampleModel and BandedSampleModel also have a 
>> meaningless override of super.equals/hashCode().
>>
>> And all of these classes suffer from casting to the specific type 
>> before verifying its class as I mentioned in the ICM.equals() review...
>>
>>             ...jim
>>
>> On 4/25/16 2:31 AM, Jayathirth D V wrote:
>>> Hi Jim,
>>>
>>> I have made changes to include check for class equality in base 
>>> class and use super.equals() from subclasses.
>>> Please find updated webrev for review :
>>> http://cr.openjdk.java.net/~jdv/8153943/webrev.02/
>>>
>>> Change related to ColorModel is present in JDK-7107905 review.
>>>
>>> Thanks,
>>> Jay
>>>
>>> -----Original Message-----
>>> From: Jim Graham
>>> Sent: Saturday, April 23, 2016 7:30 AM
>>> To: Phil Race; Jayathirth D V
>>> Cc: 2d-dev at openjdk.java.net
>>> Subject: Re: [OpenJDK 2D-Dev] Review Request for JDK-8153943 : In 
>>> java.awt.image package some of the classes are missing hashCode() or
>>> equals() method
>>>
>>> This is actually a pretty nasty issue that Joe Darcy also brought up 
>>> in the CCC review.
>>>
>>> In order to have symmetric testing of .equals(), we pretty much have 
>>> to enforce this test at all levels, including in the original
>>> ColorModels.equals() method.  If we don't enforce this in 
>>> CM.equals(), then we could run ccm.equals(othercm) and it would 
>>> return false because the class is wrong, but turning it around and 
>>> testing
>>> othercm.equals(ccm) would succeed because it doesn't enforce the 
>>> class equality.
>>>
>>> So, I'd recommend that CM.equals() tests getClass() == getClass() at 
>>> the base level and then all others will use super.equals() and get 
>>> the same protection.  It means you can't have a subclass of CCM be 
>>> "equals" to a different subclass of CCM, but that's an unfortunate 
>>> issue with equals needing to honor symmetry...  :(
>>>
>>>             ...jim
>>>
>>> On 4/20/2016 10:17 AM, Phil Race wrote:
>>>> Hi, You removed the following test in CCM.java : 2941 if
>>>> (obj.getClass() != getClass()) {
>>>> 2942 return false;
>>>>
>>>> 2943         }
>>>>
>>>> What this means is that before your change an instance of a 
>>>> subclass of CCM would never be equals() to any direct 
>>>> instantiatation of CCM but after your change it can be. I suspect 
>>>> the condition was there on purpose.
>>>>
>>>> -phil.
>>>>
>>>> On 04/20/2016 05:45 AM, Jayathirth D V wrote:
>>>>>
>>>>> Hi,
>>>>>
>>>>>
>>>>>
>>>>> _Please review the following fix in JDK9:_
>>>>>
>>>>>
>>>>>
>>>>> Bug : https://bugs.openjdk.java.net/browse/JDK-8153943
>>>>>
>>>>>
>>>>>
>>>>> This is subtask of 
>>>>> https://bugs.openjdk.java.net/browse/JDK-6588409
>>>>>
>>>>>
>>>>>
>>>>> Webrev : http://cr.openjdk.java.net/~jdv/8153943/webrev.00/
>>>>> <http://cr.openjdk.java.net/%7Ejdv/8153943/webrev.00/>
>>>>>
>>>>>
>>>>>
>>>>> Issue : Some of the java.awt.image classes are missing either
>>>>> equals() or hashCode() method.
>>>>>
>>>>>
>>>>>
>>>>> Solution : Add missing equals() or hashCode() methods.
>>>>>
>>>>>
>>>>>
>>>>> Thanks,
>>>>>
>>>>> Jay
>>>>>
>>>>>
>>>>>
>>>>
>



More information about the 2d-dev mailing list