[OpenJDK 2D-Dev] Review Request for JDK-7107905: equals() method in IndexColorModel doesnt exist and it relies on ColorModel.equals() which is not strict

[Jayathirth D V]

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 at 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.