Hi Jim,
Thanks for your valuable inputs.
Please find the updates inline. I have created new webrev
http://cr.openjdk.java.net/~jdv/7107905/webrev.01/ for review.
Are there any bugs in the database about the fact that many of these ColorModel
subclasses implement equals without hashcode? They really should both be
implemented, but since ColorModel implements the method based on its tests,
they are technically covered by the equals/hashCode contract - it's just not a
very optimal implementation of the contract.
- 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?
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.
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.
With respect to this particular equals() override I think it looks fine, though
the use of the isValid() method reduces its performance for a couple of reasons:
- it retests the index for the range [0,map size] which we already know is valid
- if the validBits is null, there is no need to even do the test.
This might be faster for the very common case:
if (validBits == null) {
if (cm.validBits != null) return false;
// loop just comparing rgb[] values
} else {
if (cm.validBits == null) return false;
// loop, testing rgb[] values and
// corresponding validBits indices directly
}
Note that in the constructor we set validBits to null if it is "true"
for all valid indices so if one of the cmaps has it null and the other does
not, then that is a very good indicator that their valid indices don't match.
- I have updated the suggested changes.
On a more minor note, I don't like the indentation of the if statement, I'd
move the "{" brace to a line of its own indented the same as its closing "}" to
make the body of the if stand out. It isn't strictly the Java coding
guidelines, but it does match a bunch of the other 2D code - sort of a local
exception to the coding style.
- 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.
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.
...jim
On 4/6/2016 4:47 AM, Jayathirth D V wrote:
> Hi,
>
> _Please review the following fix in JDK9:_
>
> Bug : https://bugs.openjdk.java.net/browse/JDK-7107905
>
> Webrev : http://cr.openjdk.java.net/~jdv/7107905/webrev.00/
>
> Issue : When we compare two IndexColorModel objects with different
> ColorMap(or any other property specific to IndexColorModel) using
> equals() method it returns true.
>
> Root cause : There is no override equals() method specific to
> IndexColorModel and it uses super class equals() which is ColorModel
> and it doesn't verify equality for IndexColorModel specific properties.
>
> Solution : Add override equals() method for IndexColorModel which
> verifies its unique properties for equality.
>
> Thanks,
>
> Jay
>
