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: [email protected]
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: [email protected]
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
