Hi Jim,
I have updated the webrev to include changes to check testBit() instead of
using isValid().
Please find the updated webrev for review:
http://cr.openjdk.java.net/~jdv/7107905/webrev.03/
Thanks,
Jay
-----Original Message-----
From: Jim Graham
Sent: Tuesday, April 12, 2016 12:21 AM
To: Jayathirth D V; Philip Race; Prasanta Sadhukhan
Cc: 2d-dev@openjdk.java.net
Subject: Re: Review Request for JDK-7107905: equals() method in IndexColorModel
doesnt exist and it relies on ColorModel.equals() which is not strict
Hi Jay,
There was one thing I pointed out in the first review that got lost in the
shuffle. When the validBits are not null you use isValid(i) to test the
values, but that method does 3 things:
- tests the index for validity, but we already know it is valid
- tests validBits for null, but we already know it is not
- calls validBits.testBit() - this is the only part we need
For performance, I'd switch to just testing for the bits directly, as in:
validBits.testBit(i) == cm.validBits.testBit(i)
...jim
On 4/11/2016 12:46 AM, Jayathirth D V wrote:
> Hi Jim,
>
> Thanks for the review.
> I have made all recommended changes and created new subtask for
> JDK-6588409(https://bugs.openjdk.java.net/browse/JDK-8153943 ).
>
> Please review updated webrev:
> http://cr.openjdk.java.net/~jdv/7107905/webrev.02/
>
> Thanks,
> Jay
>
> -----Original Message-----
> From: Jim Graham
> Sent: Friday, April 08, 2016 12:28 AM
> To: Jayathirth D V; Philip Race; Prasanta Sadhukhan
> Cc: 2d-dev@openjdk.java.net
> Subject: Re: Review Request for JDK-7107905: equals() method in
> IndexColorModel doesnt exist and it relies on ColorModel.equals()
> which is not strict
>
> Hi Jayathirth,
>
> This looks good.
>
> One thing to note for efficiency, "instanceof" also checks for null.
> It only returns true for non-null objects, so you don't need to
> explicitly test for null at the top of ICM.equals(). (Though, you
> should include a
> test(s) in the unit test that verifies that no ICM returns true for
> equals(null) to be sure.) You can see that CM.equals() is implemented this
> way.
>
> Also, for performance, ICM should include a quick "if (this == cm) return
> true;" check, like CM.equals(). I'd recommend:
>
> - first instanceof
> - then == test
> - then super.equals()
> - finally, test equality of data fields
>
> More comments inline:
>
> On 4/7/16 6:46 AM, Jayathirth D V wrote:
>> - Yes https://bugs.openjdk.java.net/browse/JDK-6588409 has mentioned
>> the same thing. Can I create a subtask to address java.awt.image changes?
>
> That would be good.
>
>> For now, it would be good to implement hashCode() on ICM now that you are
>> creating an equals() method for it.
>>
>> - I am not completely sure of what is the optimal way of adding
>> hashCode() API so I took help from internet and IDE to make the changes. I
>> am including super.hashCode() as it adds uniqueness to ICM.
>
> You did a great job. To save time in the future, you should all have copies
> of the latest version of "Effective Java" by Josh Bloch. It has a whole
> chapter on equals/hashCode. It's a very handy reference for all sorts of
> good programming practice for the Java language.
>
>> Also, ColorModel.hashCode() is a poor implementation. It doesn't use the
>> paradigms recommended by Effective Java and looks like it produces poor
>> hashes as a result (just in the first two elements of the hashCode I see a
>> collision due to poor choice of numbers).
>> - I think since we are not using Prime numbers and it might result in
>> improper hash code. I have made similar changes to hashCode() of ColorModel.
>
> Looks great.
>
>> - In the same file itself we are following Java coding guidelines of
>> having braces after if like "if () {". I am not completely sure of including
>> "{" in new line.
>
> You are correct, that matching local code is a good consideration when
> considering code style. In this case, though, the indentation on these if
> statements is an example of what we're trying to avoid so it would be better
> to fix these particular statements (don't bother fixing the other lines in
> the file at this point, that can wait until we have to update other parts of
> the file, but don't propagate a bad style in new code).
> In particular:
>
> Never do this:
>
> if (some complex test ||
> some additional tests ||
> some additional tests ||
> some additional tests ||
> some continuation of the test) {
> the body of the code;
> more body of the code;
> }
> Quick question - where does the body of the if statement start? Does your
> eye see it in a fraction of a second or do you have to search for it?
>
> That is the worst option for indenting an if statement with continuation
> lines. Never do that in new code. Do either of the following two versions:
>
> Java Code Style guidelines recommends indenting 8 spaces for
> conditional
> continuations:
> if (some complex test ||
> some additional tests ||
> some additional tests ||
> some additional tests ||
> some continuation of the test) {
> the body of the code;
> more body of the code;
> }
>
> Jim's personal extension to the JCS recommends (and the 2D team historically
> tended to agree):
> if (some complex test ||
> some additional tests ||
> some additional tests ||
> some additional tests ||
> some continuation of the test)
> {
> the body of the code;
> more body of the code;
> }
>
> Both of those immediately draw the eye to the separating point between the
> conditional and the body of the code.
>
>> I'd also add a few test cases that test that 2 separately and identically
>> constructed ICM instances are equals() == true, tested with one of each of
>> the constructors...
>>
>> - I have made changes to test case for verifying all constructors with
>> same ICM. Also added verification for hashCode value.
>
> Unfortunately, some of your tests for hashCode make an invalid assumption.
> It is technically correct for the hash codes of 2 different objects to be the
> same regardless of whether they are equals() or not.
> In fact, a perfectly valid implementation of hashCode() could return a
> constant number and it would be correct from the perspective of the
> equals/hashCode contract. (Such code, however, would not be optimal, but it
> would be valid/correct-to-spec.) The only condition that is required is that
> the hash codes match if the objects are equals(), but they are allowed to
> match if the objects are !equals(). In other words:
>
> boolean equals1 = (o1.equals(o2));
> boolean equals2 = (o2.equals(o1));
> boolean equalsH = (o1.hashCode() == o2.hashCode());
>
> if (equals1 != equals2) that is an error - we should test this if
> (equals1 && !equalsH) that is an error - we should test this // No
> other conditions are an error, no other testing should be done
>
> In particular, the cases where you test the hash codes for being the same on
> objects that are not intended to be equals() should be deleted.
> It would be good to also add tests to make sure that they are
> symmetrically equals (or symmetrically not equals) as well (i.e.
> o1.equals(o2) matches o2.equals(o1) in all cases whether they match or not).
>
> Since it is less than optimal for hash codes to match if the objects are not
> equal, it might potentially be something to check on, but it should not
> generate a unit test failure and so should not appear in the unit test for
> this bug. Such a "code collision test" would be part of a performance test
> run periodically for QA, but we have never bothered with hashCode
> optimization unless a customer submits a complaint about the performance of
> some object in a hash map due to hash collisions (and I doubt that ColorModel
> objects are being used in such a manner), so I wouldn't recommend it here.
>
> ...jim
>
