[OpenJDK 2D-Dev] Review Request for JDK-7107905: ColorModel subclasses are missing hashCode() or equals() or both methods

[Jim Graham]

I think we should move this issue (array size returned from getCompSizes) into a separate bug entry and a separate fix. 
I don't think we need to fix the clone() in the constructor and the getter just to get hashcode/equals right...

			...jim

On 1/31/17 2:34 PM, Jim Graham wrote:
> For an application to run into this same issue they'd have to expect getCompSizes() to return data for components that
> don't exist.  It's unlikely they would use that data if they really understand the objects.  While that would be odd, I
> guess I can see someone might be constructing all of their CM's from an array of 4 components regardless of the number
> of actual components and we'd be happily remembering the useless extra components and returning an array of 4 from
> getCompSizes().  As I said, they shouldn't really be reading and interpreting those extra components for any image
> processing, but I can imagine that they might do something like create a variant CM by calling the CompSizes() and
> copying them into a new array to construct a new CM with modifications.  They might just robotically always copy 4
> values without really checking how many are valid.  That's a stretch, and their code is weak.  I can conceive of how
> this might happen, but I really have no idea how likely it is...
>
>             ...jim
>
> On 1/30/17 3:56 PM, Phil Race wrote:
>> Sounds like we should at least try to get the tests updated so they only test what the spec. says.
>> Although it does indicate that there is at least a chance that application code might also fail due to similar
>> assumptions.
>> Does #1 not fail with the previous iteration of this change too ?
>>
>> -phil.
>>
>> On 01/30/2017 01:40 PM, Jim Graham wrote:
>>> Hmmm.  Sounds like the test cases were written based on bugs in the implementation.  I'm not sure what the best tactic
>>> is here for the short term for getting this in, but many of these changes should eventually be considered bugs in the
>>> tests.  Is it acceptable to break API tests like this at the last minute even if the tests are at fault?  Phil?
>>>
>>> Notes on specific instances below...
>>>
>>> On 1/30/17 2:22 AM, Jayathirth D V wrote:
>>>> Hi Phil,
>>>>
>>>>     1)api/java_awt/Image/ColorModel/index.html#Constructor: Failed. test cases: 4; passed: 3; failed: 1; first test
>>>> case failure: ColorModel2001
>>>>
>>>>     This test fails because getComponentSize() returned an array with length 3 but it expects the length to be 4. In
>>>> the test case they have bits per component array     of length 4 like {8, 8, 8, 8}. But in the test case wherever
>>>> they are passing "has Alpha" as "false" we omit the alpha component bit. This is because of tighter check     that we
>>>> have in ColorModel class as "nBits = Arrays.copyOf(bits, numComponents);" . "numComponents" will be 3 if hasAlpha is
>>>> false.
>>>
>>> This is a bug in the test then, especially if the size of our array matches the return value of getNumComponents.
>>>
>>>>     2)api/java_awt/Image/ColorModel/index.html#Equals: Failed. test cases: 3; passed: 2; failed: 1; first test case
>>>> failure: ColorModel0004
>>>>
>>>>     Here they check for equality between 2 ColorModel objects having same values, but it fails because now we are
>>>> using identity-as-equals check in ColorModel.
>>>
>>> How do they accomplish this when the CM class is abstract?  Do they create a relatively empty subclass and instantiate
>>> that?
>>>
>>> The documentation for the equals() method does not document the conditions under which it returns true, it uses a
>>> vague concept of "equals this ColorModel".  I don't see how they could test anything given that documentation.
>>>
>>>>     3)api/java_awt/Image/ColorModel/index.html#HashCode: Failed. test cases: 2; passed: 1; failed: 1; first test case
>>>> failure: ColorModel2006
>>>>
>>>>     Here they check for hashCode equality between 2 ColorModel objects having same values, but it fails since we
>>>> don't have hashCode check in ColorModel and it     will be different between 2 Objects.
>>>
>>> Same as above, there are no promises documented.
>>>
>>>>     4)api/java_awt/Image/ComponentColorModel/index.html#ConstructorTesttestCase1: Failed. test cases: 2; passed: 1;
>>>> failed: 1; first test case failure: testCase1
>>>>
>>>>     Throws "java.lang.ArrayIndexOutOfBoundsException: 3". This is also happening because of same reason as why the
>>>> first JCK test is failing. We omit alpha bit if     hasAlpha is false but JCK test tries to call getComponentSize()
>>>> with index 3 which throws ArrayIndexOutOfBoundsException.
>>>
>>> Same assessment as #1 above...
>>>
>>> Again, while these are my recommendations about the correctness of these tests, the question remains whether we want
>>> to introduce a change at this point in the release cycle that will essentially invalidate a number of tests that have
>>> been working for several releases already.  I'll leave that tactic issue to Phil...
>>>
>>>                 ...jim
>>>
>>