So +1 .. and please update the CCC
-phil.
On 2/9/17, 8:51 PM, Jayathirth D V wrote:
Hi,
Phil & Jim thanks for your review.
I have modified the code to remove the unneeded import of import
java.util.Objects.
Please find the updated webrev :
Hi,
Phil & Jim thanks for your review.
I have modified the code to remove the unneeded import of import
java.util.Objects.
Please find the updated webrev :
http://cr.openjdk.java.net/~jdv/7107905/webrev.19/
Thanks,
Jay
From: Phil Race
Sent: Friday, February 10, 2017 3:35 AM
Oh .. my reply was to an off-list email. I did not notice that.
So I should repeat that here :
On 2/9/17 12:38 PM, Phil Race wrote:
32 import java.util.Objects;
This is now un-used, isn't it ? Yet all 3 subclasses still have this
import.
I don't need to "approve" a new webrev containing
From my end this looks good. +1 except for 2 outstanding review issues:
- Would like to hear back final comments from Joe Darcy on the new doc
changes/CCC request
- Phil pointed out that there is an unneeded import in some of the files. I agree that we should make a final webrev to
delete
Hello All,
There was a closed test which was failing because of identity-as-equals
approach for ColorModel equals() method.
I have modified it and added in the webrev. Along with this we are now using
colorspace.hashCode() in hashCode() functions instead of
Objects.hashCode(this.colorspace).
Hello All,
I have updated the webrev to include the following changes.
1) Have identity as equals check in equals() method of ColorModel but
elaborate the specification of equals() and hashCode() in ColorModel on what
properties to check in subclasses of ColorModel.
I don't think there is a spec anywhere that 2 objects created by the same constructor must evaluate as equals(). In
particular, 2 instances of Object created with the null constructor do not evaluate as equals(). So, with respect to
testing a spec, they are reading a promise into the base
Hi Phil & Jim,
Thanks for your inputs.
I have verified with previous iteration of the changes and both #1 and #4 fail
from the time we have introduced the change in how we calculate "nBits" in
ColorModel.
-nBits = bits.clone();
+/*
+ * We need significant bits value
Sounds like we should at least try to get the tests updated so they only
test what the spec. says.
Although it does indicate that there is at least a chance that
application code might also fail due to similar assumptions.
Does #1 not fail with the previous iteration of this change too ?
Hi Phil,
Thanks for your review, I have updated the webrev to include your suggestions :
http://cr.openjdk.java.net/~jdv/7107905/webrev.15/
I just noticed that some JCK test are failing because of this change :
1)api/java_awt/Image/ColorModel/index.html#Constructor: Failed. test
Hi Jay,
Joe had suggested
>ColorModel is redefined to use identity-is-equality equals semantics,
the default behavior from object.
> This avoids the need to define a hashcode method in the base class.
However you have restored the problem that started this.
ColorModel now over-rides equals
Hello All,
After getting feedback from Joe in CCC I have made changes to the code to
reflect Joe's suggestions.
For ColorModel class I have just added identity-is-equality equals().
For its subclasses like IndexColorModel, PackedColorModel &
ComponentColorModel, I have added instanceOf checks
Looks good to me...
...jim
On 07/20/2016 09:53 AM, Jayathirth D V wrote:
Hi Jim,
Thanks for your input.
I have updated specification of ColorModel and its subclasses.
Please find new webrev for review:
http://cr.openjdk.java.net/~jdv/7107905/webrev.13/
Thanks,
Jay
OK by me.
-phil.
On 07/20/2016 09:53 AM, Jayathirth D V wrote:
Hi Jim,
Thanks for your input.
I have updated specification of ColorModel and its subclasses.
Please find new webrev for review:
http://cr.openjdk.java.net/~jdv/7107905/webrev.13/
Thanks,
Jay
-Original Message-
From: Jim
Hi Jay,
When I write javadoc suggestions I sometimes use the short-hand "{some text}" to indicate that it should be formatted
using javadoc protocols, typically that expands to "{@code some text}", but sometimes a link may be appropriate. It
looks like you copied and pasted a number of places
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
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
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
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/
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
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
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
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.
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,
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
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.
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
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.
Also, I'm not sure == is a good way to compare ColorSpace instances - Phil?
...jim
On 05/29/2016
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
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
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:
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
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:
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
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:
35 matches
Mail list logo
