On 05/31/2016 12:16 PM, Jim Graham wrote:
Hi Phil,
I don't think that alternate wording conveys that this is not a case
of "instanceof" and is instead a case of "getClass() =="...
Perhaps we don't need "exact", but "type" is vague enough to call into
question how we do the test. I could see "the same class (and not a
subclass) as this"...?
That wording works for me. I just thought someone else might prefer
'type' strongly enough to ask for it.
-phil.
...jim
On 05/31/2016 11:56 AM, Phil Race wrote:
1449 * the target object must be the exact same class as this
I believe the language people like to say "the same type" so maybe say
1449 * the target object must be of the same type (i.e. class)
as this
Could IndexColorModel cache the hash code since it could be fairly
expensive to compute ?
Arrays.hashCode() is the expensive bit.
Less so PackedColorModel but could be considered there too.
No other comments at this time.
-phil.
On 05/29/2016 11:55 PM, Jayathirth D V wrote:
Hi Jim,
Thanks for your valuable inputs. I have made recommended changes.
Please find updated webrev for review:
http://cr.openjdk.java.net/~jdv/7107905/webrev.08/
Thanks,
Jay
-----Original Message-----
From: Jim Graham
Sent: Friday, May 27, 2016 11:52 PM
To: Jayathirth D V; Philip Race
Cc: 2d-dev@openjdk.java.net
Subject: Re: Review Request for JDK-7107905: ColorModel subclasses are
missing hashCode() or equals() or both methods
I think that makes sense. If it has no type-specific information to
compare, then it doesn't need to override them...
...jim
On 05/27/2016 05:32 AM, Jayathirth D V wrote:
Hi Jim,
Since we will be moving comparison for ColorSpace and transferType to
CM, can we remove equals() & hashCode() methods in CCM?
For PCM and ICM we have unique variables which we can compare, there
will be no problem.
Thanks,
Jay
-----Original Message-----
From: Jim Graham
Sent: Friday, May 27, 2016 2:18 PM
To: Jayathirth D V; Philip Race
Cc: 2d-dev@openjdk.java.net
Subject: Re: Review Request for JDK-7107905: ColorModel subclasses are
missing hashCode() or equals() or both methods
Hi Jay,
.equals() should not be comparing any fields that are computed from
other fields - generally fields that come directly from a constructor
argument tend to be the fields to compare. Internal state variables
(such as "isInited" and the like) should never be compared. It
should be comparing fields that affect the interpretation of the
color. To that end...
In CM, add comparison/hash of:
ColorSpace
transferType
In CCM, do not compare or hash:
needScaleInit (internal state for lazy ScaleInit)
noUnnorm (computed from other constructor arguments)
nonStdScale (computed from other constructor arguments)
signed (computed from other constructor arguments)
transferType (tested in CM)
(basically, CCM doesn't have anything to compares as all of its
significant values are compared in the super class)
...jim
On 05/27/2016 01:02 AM, Jayathirth D V wrote:
Hi,
Gentle reminder.
Please review updated fix:
http://cr.openjdk.java.net/~jdv/7107905/webrev.07/
Thanks,
Jay
-----Original Message-----
From: Jayathirth D V
Sent: Thursday, May 19, 2016 6:43 PM
To: Philip Race; Jim Graham
Cc: 2d-dev@openjdk.java.net
Subject: Review Request for JDK-7107905: ColorModel subclasses are
missing hashCode() or equals() or both methods
Hi,
Previously for this bug we were making changes related only to
IndexColorModel. Since we are expanding to include hashCode() or
equals() method from PackedColorModel and ComponentColorModel, I
have created single webrev for review under the same bug id.
Now the "getclass()==" check is present in base class ColorModel and
each subclass of ColorModel have their own equality check for
properties.
Details:
Bug : https://bugs.openjdk.java.net/browse/JDK-7107905
Updated Webrev : http://cr.openjdk.java.net/~jdv/7107905/webrev.07/
Please review the changes at your convenience.
Thanks,
Jay
