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

[Sergey Bylokhov]

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.