'Re: [OpenJDK 2D-Dev] Review Request for JDK-8153943 : In PixelInterLeavedSampleModel and BandedSampl'

[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-2d-dev
Subject:    Re: [OpenJDK 2D-Dev] Review Request for JDK-8153943 : In PixelInterLeavedSampleModel and BandedSampl
From:       Jim Graham <james.graham () oracle ! com>
Date:       2016-06-29 22:51:57
Message-ID: 5774510D.1020307 () oracle ! com
[Download RAW message or body]

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/share/classes/java/awt/image/BandedSampleModel.java.sdiff.html
>  
> http://cr.openjdk.java.net/~jdv/8153943/webrev.02/src/java.desktop/share/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: 2d-dev@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@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
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > 
> > > > > 
> 


[prev in list] [next in list] [prev in thread] [next in thread]
Configure | About | News | Add a list | Sponsored by KoreLogic