Sure Sergey after technical review is done I will raise CCC for the same. Thanks, Jay
-----Original Message----- From: Sergey Bylokhov Sent: Friday, April 08, 2016 12:42 AM To: Jim Graham; Jayathirth D V; Philip Race; Prasanta Sadhukhan Cc: 2d-dev@openjdk.java.net Subject: Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905: equals() method in IndexColorModel doesnt exist and it relies on ColorModel.equals() which is not strict Small note that we should not forget to create a ccc. On 07.04.16 21:58, Jim Graham wrote: > 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 -- Best regards, Sergey.
