[OpenJDK 2D-Dev] Review Request for JDK-8153943 : In PixelInterLeavedSampleModel and BandedSampleModel we dont need hashCode() method

Jim Graham james.graham at oracle.com
Tue Jun 28 20:40:36 UTC 2016 

Still hoping to hear an opinion from Phil on this...

The alternative is to add equals() overrides in the subclasses that just do "obj instanceof <myclass> && super.equals()" 
which would only matter in some specific somewhat degenerate cases.

The intent would be that even though the layout and pixel fetching behavior for a specific configuration of PISM and BSM 
are identical, they are philosophically not the same and so should not evaluate as equals()...

...Or, should they?

			...jim

On 6/27/16 4:05 PM, Jim Graham wrote:
> This is odd that two completely different classes have identical "equals()" methods.  After looking into it in more
> detail it looks as if ComponentSM is implemented as a general case that can encompass either PixelInterleaved or Banded
> pixel layouts - which means the subclasses are mostly just cosmetic (offering the constructors that make most sense if
> the pixels are laid out in the different ways) and only Banded offers a different implementation of getDataElements
> which is only different from the super implementation by virtue of eliminating a "multiply by a number which we know to
> be 1".
>
> There are also some restrictions in the constructors that enforce limits on the various values that CSM allows in its
> general implementation, so it isn't possible to use the PixelInterleaved constructor to create an arbitrarily-valued CSM
> nor to use the Banded constructors for the same purpose.  The overlap in the type of CSM you can create from each of
> their constructors is very tiny to non-existant.
>
> The end result is that it ends up being infeasible to define a PixelInterleaved and a Banded SM that are equals() (not
> impossible, but you'd have to have a very degenerate case like a 1x1 image to make it through the various restrictions
> in the constructors and the offsets and the scanline strides and pixel strides, etc.).  It's really odd in theory, and
> while not a problem in practice it still feels as if it violates an expectation.  The primary restrictions I see getting
> in the way of different classed objects being equals() is that Banded forces a pixel stride of 1 and PixelInterleaved
> enforces that all band offsets are smaller than the scan stride.
>
> So, technically, I don't see any issue with just removing the hash method as the webrev proposes, but I'd like to see
> Phil's reaction to the situation we are in here with respect to intent vs. theory vs. practice vs. developer expectation...
>
>                 ...jim
>
> On 6/24/16 10:34 AM, Jayathirth D V wrote:
>> Hi,
>>
>> Please find following changes for review in JDK9 :
>>
>> Bug : https://bugs.openjdk.java.net/browse/JDK-8153943
>>
>> Webrev : http://cr.openjdk.java.net/~jdv/8153943/webrev.03/
>>
>> Issue : We have hashCode() method in PixelInterleavedSampleModel and BandedSampleModel, but we don't have
>> corresponding equals() method.
>>
>> Solution : In PixelInterleavedSampleModel and BandedSampleModel we don't have unique properties that are specific to
>> these subclasses and we have proper equals() & hashCode() method in parent class ComponentSampleModel. So removed
>> hashCode() method present in PixelInterleavedSampleModel and BandedSampleModel.
>>
>> 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