These classes are a mess.
CSM has numBands, but so does the super class. We should probably
delete the copy in the subclass.
CSM constructors re-initialize data members that were already
initialized in the super() constructor.
CSM.equals() and hashCode() include code that really should belong in
SM.equals() and SM.hashCode(), but SM doesn't override those.
CSM constructor comments talk about exceptions thrown for things like
"numBands" even though there is no such argument in the constructor -
the number of bands is inferred from the number of band offsets, so it
should really be expressed in terms of that array length instead.
We should follow the same pattern as CM, have SM override equals/hash
and calculate its own values. Have the subclasses stop duplicating
fields and initialization steps that would be found in the super class,
etc...
...jim
On 07/28/2016 04:42 AM, Jayathirth D V wrote:
Hi,
We can follow the same approach of what we did in ColorModel.equals() for
JDK-7107905.
We can use getClass() check in ComponentSampleModel.equals() to differentiate
between BandedSampleModel and PixelInterleavedSampleModel in rare cases where they
can have same values and remove the need for equals() & hashCode() methods in
these subclasses.
We can update the specification of ComponentSampleModel.equals() to mention
what properties we are checking and what its subclasses to follow as we did in
ColorModel.
Please find webrev for the above changes:
http://cr.openjdk.java.net/~jdv/8153943/webrev.04/
Thanks,
Jay
-----Original Message-----
From: Jim Graham
Sent: Thursday, June 30, 2016 4:22 AM
To: Phil Race
Cc: Jayathirth D V; [email protected]
Subject: Re: [OpenJDK 2D-Dev] Review Request for JDK-8153943 : In
PixelInterLeavedSampleModel and BandedSampleModel we dont need hashCode() method
Hi Phil,
The following 2 statements are factually correct ignoring what we plan to do
about these classes:
- If they don't override equals() then they shouldn't override hash
- Their raw casts were wrong (and the fact that nobody ever noticed that is a
good indication that these methods have probably not been used in the wild).
What I'm waffling on is that while the equals() methods appeared functionally
useless, in practice they can either block or allow disparate class objects
from being equal to each other and I'm thinking that one could argue that it
makes strategic sense to enforce the correct class type (using instanceof) to
match the behavioral expectations of a developer.
Since I sent that out I also checked a similar case in the Number subclasses
and an Integer and a Long will not evaluate to equals even if they hold the
same cardinal value. Arguably that's even a stronger case of when 2 disparate
objects might want to be seen as equals(), but we don't allow it there.
Arguably, the override of hash is still unnecessary since it would be fine to allow
disparate classes to have the same hash value from the super class - that doesn't violate
any contracts. You can't allow it to inherit from Object.hashCode() since that hash
value is very strict, but it would be OK to override the somewhat "property
oriented" hash code from their immediate superclass. I would lean towards leaving
it in if we decide to override equals() just to avoid the argument.
Given the fact that these equals() methods have obviously not been used much, I don't have a strong
opinion between "saving code by just not overriding them" and "making the different
subclasses have unique identities by overriding and preventing them from being equal to each
other".
In the end, if we do fix the equals() for these classes, we should document why
they are overriding equals() even if it appears to do not really value added
testing compared to the super class...
...jim
On 06/29/2016 02:29 PM, Phil Race wrote:
To remind myself and others .. this review started out with changes to
the ColorModel classes mixed in.
Those were separated out making it easier to review just the
SampleModel hierachy here.
Jim observed (see some way below for the context) :-
> PixelInterleavedSampleModel and BandedSampleModel also have a >
meaningless override of super.equals/hashCode().
I think that comment is from when the webrev for those files looked
like this :-
http://cr.openjdk.java.net/~jdv/8153943/webrev.02/src/java.desktop/sha
re/classes/java/awt/image/BandedSampleModel.java.sdiff.html
http://cr.openjdk.java.net/~jdv/8153943/webrev.02/src/java.desktop/sha
re/classes/java/awt/image/PixelInterleavedSampleModel.java.sdiff.html
So Jim, are you suggesting a preferred option is to go back to that
proposal - at least for the equals methods ?
[Caveat : the proposed equals method there should test instanceof
before doing a cast but I actually don't know why it needs the cast
anyway]
The hashCode() could be left alone in that case.
But if the equals methods are not added as there, then I think we do
need to remove the
hashCode() since it could be different even if (albeit only in a
degenerate case), whilst instances (of the different classes) could
compare as equal.
But in either case we need to look to the super-class which is lacking
any documentation of its own describing what makes two instances
equal.
We could try to explain there what might otherwise be surprising.
-phil.
On 06/28/2016 01:40 PM, Jim Graham wrote:
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
