Hi Jay,
We need to reference the properties from the base class in the
subclasses. We don't need to list them every time, but we could
introduce the "additional properties" using something like:
"... we check all of the properties checked by the {equals} method of
{ColorModel}, and the following additional properties:"
Arguably, we could omit the class comparison text from the subclasses as
well by using a reference like that, but I think that the class
equivalence test is enough out of the ordinary that it does bear
reiterating it in every subclass as you already do now even though we
only reference the specific other properties tested by a reference.
A few grammar fixes:
In all of the classes, insert a space before any parentheses in comment
text as in "the same class (and not a subclass)". (That wouldn't apply
if the text in the comment is referring to code - then space it as you
would actual code.)
In CM, insert the word "the" as in "also check the following ..."
In ICM I would refer to the "Valid bits of" instead as "The list of
valid pixel indices in".
In PCM I would change "check maskArray of ..." to "check the masks of
the ..." and pluralize the word "samples".
...jim
On 06/29/2016 04:13 AM, Jayathirth D V wrote:
Hi,
Since Joe mentioned to add information related to what fields we are using to
calculate equals() method in ColorModel and its subclasses I have updated the
spec for the same.
Please find updated webrev for review:
http://cr.openjdk.java.net/~jdv/7107905/webrev.11/
Thanks,
Jay
-----Original Message-----
From: Jim Graham
Sent: Saturday, June 04, 2016 4:52 AM
To: Jayathirth D V; Philip Race
Cc: 2d-dev@openjdk.java.net
Subject: Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905: ColorModel
subclasses are missing hashCode() or equals() or both methods
That looks good to me. Has the CCC cleared yet?
...jim
On 6/3/16 2:49 AM, Jayathirth D V wrote:
Hi Jim,
Oh that's an important change.
Thanks for your review.
I have updated ColorModel constructor to copy nBIts only of numOfComponents
length and I have removed validBits check in hashCode() of ICM.
Please find updated webrev for review:
http://cr.openjdk.java.net/~jdv/7107905/webrev.10/
Thanks,
Jay
-----Original Message-----
From: Jim Graham
Sent: Friday, June 03, 2016 2:25 AM
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 just noticed a hashCode/equals violation that we created a few revisions ago.
We compute the hash of the validBits in ICM, but we only compare validBits up
to the number of colors in the colormap. One could construct two ICMs that
have different validBits that are identical in the first N bits (N = number of
colors), but have different bits beyond that, and those 2 ICMs would evaluate
as equals(), but their hashcodes would be different.
Possible solutions:
- Truncate validBits when it is accepted in the constructor
(validBits.and(...))
- Manually compute the hash of the first N bits of validBits
- Not use validBits in the hash code since it is allowed to omit
significant data from the hash
(Note that everything in hashcode must participate in equals(), but
not vice versa)
The same is true of nBits in ColorModel. (nBits can be copied and
truncated with Arrays.copyOf)
The same is *not* true of maskBits in PCM since the constructor creates an
array of the precise length it needs...
...jim
On 6/2/16 7:07 AM, Jayathirth D V wrote:
Hi Phil,
I have updated the code with all the changes I mentioned previously. I am
caching hashCode when first time hashCode() is called.
Please find the updated webrev for review:
http://cr.openjdk.java.net/~jdv/7107905/webrev.09/
Thanks,
Jay
-----Original Message-----
From: Philip Race
Sent: Wednesday, June 01, 2016 10:06 PM
To: Jayathirth D V
Cc: Jim Graham; 2d-dev@openjdk.java.net
Subject: Re: Review Request for JDK-7107905: ColorModel subclasses
are missing hashCode() or equals() or both methods
Please post the updated webrev.
-phil.
On 6/1/16, 12:02 AM, Jayathirth D V wrote:
Hi Phil& Jim,
Collating all the changes needed:
1) I have removed both equals()& hashCode() in CCM but forgot to add the file
while creating webrev. I will include changes in CCM in updated webrev.
2) Caching of hashCode value in IndexColorModel& PackedColorModel :
We can cache hashCode value when constructor is called or when
hashCode() is called for first time. What approach we have to follow?
3) Comment section of equals() method, I will update it to :
1449 * the target object must be the same class (and not a
subclass) as this
4) I will use .equals() to compare colorSpace values in CM.equals() so it will
be like we are not intending ColorSpace class to never override equals() method.
Please let me know your inputs.
Thanks,
Jay
-----Original Message-----
From: Jim Graham
Sent: Wednesday, June 01, 2016 4:14 AM
To: Phil Race
Cc: Jayathirth D V; 2d-dev@openjdk.java.net
Subject: Re: Review Request for JDK-7107905: ColorModel subclasses
are missing hashCode() or equals() or both methods
I think we should use .equals() here, otherwise it looks like a recommendation
that the intent is for those classes to never implement it...
...jim
On 05/31/2016 03:31 PM, Phil Race wrote:
I don't know of any design intent, so yes, I meant more as a
practical matter.
Unless they subclass then using equals() which I thought unlikely
it makes no difference here.
But it would be proof against that to use equals, unlikely to
matter as it might be ..
-phil.
On 05/31/2016 03:19 PM, Jim Graham wrote:
On 05/31/2016 02:50 PM, Phil Race wrote:
On 05/31/2016 12:20 PM, Jim Graham wrote:
Hi Jay,
You were going to remove hashCode/equals from CCM, but instead
you simply removed it from the patch. You still need to edit it
to remove the existing methods.
Oh yeah ! CCM is gone from the latest webrev. I expect that was
not intentional.
Also, I'm not sure == is a good way to compare ColorSpace
instances
- Phil?
Neither ColorSpace nor ICC_ColorSpace over-ride equals or
hashCode and all the predefined ones are constructed as
singletons so it seems unlikely to cause problems
Should it use .equals() on principle, though? Otherwise we are
baking in the assumption that it doesn't implement equals() into
our implementation of the CM.equals() method. Might it some day
implement equals (perhaps it isn't a practical issue today, but it
might be in the future when we forget that it was omitted in this
usage we create today).
I guess what I'm asking is if it is a design feature that
different objects are considered non-equal (such as an object
that, for instance, has only predetermined instances that are
shared by reference and reused). I think color spaces are sort of
in-between in that we define a few constants that simply get used
by reference in 99% of cases, but that isn't a design feature,
more like a practical issue...
...jim