Review Request for JDK-7107905: ColorModel subclasses are missing hashCode() or equals() or both methods

classic Classic list List threaded Threaded
44 messages Options
123
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Review Request for JDK-7107905: ColorModel subclasses are missing hashCode() or equals() or both methods

Jayathirth D v
Hi,

Previously for this bug we were making changes related only to IndexColorModel. Since we are expanding to include hashCode() or equals() method from PackedColorModel and ComponentColorModel, I have created single webrev for review under the same bug id.

Now the "getclass()==" check is present in base class ColorModel and each subclass of ColorModel have their own equality check for properties.

Details:
Bug : https://bugs.openjdk.java.net/browse/JDK-7107905 
Updated Webrev : http://cr.openjdk.java.net/~jdv/7107905/webrev.07/ 

Please review the changes at your convenience.

Thanks,
Jay
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Review Request for JDK-7107905: ColorModel subclasses are missing hashCode() or equals() or both methods

Jayathirth D v
Hi,

Gentle reminder.
Please review updated fix:
http://cr.openjdk.java.net/~jdv/7107905/webrev.07/

Thanks,
Jay

-----Original Message-----
From: Jayathirth D V
Sent: Thursday, May 19, 2016 6:43 PM
To: Philip Race; Jim Graham
Cc: [hidden email]
Subject: Review Request for JDK-7107905: ColorModel subclasses are missing hashCode() or equals() or both methods

Hi,

Previously for this bug we were making changes related only to IndexColorModel. Since we are expanding to include hashCode() or equals() method from PackedColorModel and ComponentColorModel, I have created single webrev for review under the same bug id.

Now the "getclass()==" check is present in base class ColorModel and each subclass of ColorModel have their own equality check for properties.

Details:
Bug : https://bugs.openjdk.java.net/browse/JDK-7107905 
Updated Webrev : http://cr.openjdk.java.net/~jdv/7107905/webrev.07/ 

Please review the changes at your convenience.

Thanks,
Jay
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Review Request for JDK-7107905: ColorModel subclasses are missing hashCode() or equals() or both methods

Jim Graham-5
Hi Jay,

.equals() should not be comparing any fields that are computed from
other fields - generally fields that come directly from a constructor
argument tend to be the fields to compare.  Internal state variables
(such as "isInited" and the like) should never be compared.  It should
be comparing fields that affect the interpretation of the color.  To
that end...

In CM, add comparison/hash of:
    ColorSpace
    transferType

In CCM, do not compare or hash:
    needScaleInit (internal state for lazy ScaleInit)
    noUnnorm (computed from other constructor arguments)
    nonStdScale (computed from other constructor arguments)
    signed (computed from other constructor arguments)
    transferType (tested in CM)
(basically, CCM doesn't have anything to compares as all of its
significant values are compared in the super class)

                        ...jim

On 05/27/2016 01:02 AM, Jayathirth D V wrote:

> Hi,
>
> Gentle reminder.
> Please review updated fix:
> http://cr.openjdk.java.net/~jdv/7107905/webrev.07/
>
> Thanks,
> Jay
>
> -----Original Message-----
> From: Jayathirth D V
> Sent: Thursday, May 19, 2016 6:43 PM
> To: Philip Race; Jim Graham
> Cc: [hidden email]
> Subject: Review Request for JDK-7107905: ColorModel subclasses are missing hashCode() or equals() or both methods
>
> Hi,
>
> Previously for this bug we were making changes related only to IndexColorModel. Since we are expanding to include hashCode() or equals() method from PackedColorModel and ComponentColorModel, I have created single webrev for review under the same bug id.
>
> Now the "getclass()==" check is present in base class ColorModel and each subclass of ColorModel have their own equality check for properties.
>
> Details:
> Bug : https://bugs.openjdk.java.net/browse/JDK-7107905
> Updated Webrev : http://cr.openjdk.java.net/~jdv/7107905/webrev.07/
>
> Please review the changes at your convenience.
>
> Thanks,
> Jay
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Review Request for JDK-7107905: ColorModel subclasses are missing hashCode() or equals() or both methods

Jayathirth D v
Hi Jim,

Since we will be moving comparison for ColorSpace and transferType to CM, can we remove equals() & hashCode() methods in CCM?
For PCM and ICM we have unique variables which we can compare, there will be no problem.

Thanks,
Jay

-----Original Message-----
From: Jim Graham
Sent: Friday, May 27, 2016 2:18 PM
To: Jayathirth D V; Philip Race
Cc: [hidden email]
Subject: Re: Review Request for JDK-7107905: ColorModel subclasses are missing hashCode() or equals() or both methods

Hi Jay,

.equals() should not be comparing any fields that are computed from other fields - generally fields that come directly from a constructor argument tend to be the fields to compare.  Internal state variables (such as "isInited" and the like) should never be compared.  It should be comparing fields that affect the interpretation of the color.  To that end...

In CM, add comparison/hash of:
    ColorSpace
    transferType

In CCM, do not compare or hash:
    needScaleInit (internal state for lazy ScaleInit)
    noUnnorm (computed from other constructor arguments)
    nonStdScale (computed from other constructor arguments)
    signed (computed from other constructor arguments)
    transferType (tested in CM)
(basically, CCM doesn't have anything to compares as all of its significant values are compared in the super class)

                        ...jim

On 05/27/2016 01:02 AM, Jayathirth D V wrote:

> Hi,
>
> Gentle reminder.
> Please review updated fix:
> http://cr.openjdk.java.net/~jdv/7107905/webrev.07/
>
> Thanks,
> Jay
>
> -----Original Message-----
> From: Jayathirth D V
> Sent: Thursday, May 19, 2016 6:43 PM
> To: Philip Race; Jim Graham
> Cc: [hidden email]
> Subject: Review Request for JDK-7107905: ColorModel subclasses are
> missing hashCode() or equals() or both methods
>
> Hi,
>
> Previously for this bug we were making changes related only to IndexColorModel. Since we are expanding to include hashCode() or equals() method from PackedColorModel and ComponentColorModel, I have created single webrev for review under the same bug id.
>
> Now the "getclass()==" check is present in base class ColorModel and each subclass of ColorModel have their own equality check for properties.
>
> Details:
> Bug : https://bugs.openjdk.java.net/browse/JDK-7107905
> Updated Webrev : http://cr.openjdk.java.net/~jdv/7107905/webrev.07/
>
> Please review the changes at your convenience.
>
> Thanks,
> Jay
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Review Request for JDK-7107905: ColorModel subclasses are missing hashCode() or equals() or both methods

Jim Graham-5
I think that makes sense.  If it has no type-specific information to
compare, then it doesn't need to override them...

                        ...jim

On 05/27/2016 05:32 AM, Jayathirth D V wrote:

> Hi Jim,
>
> Since we will be moving comparison for ColorSpace and transferType to CM, can we remove equals() & hashCode() methods in CCM?
> For PCM and ICM we have unique variables which we can compare, there will be no problem.
>
> Thanks,
> Jay
>
> -----Original Message-----
> From: Jim Graham
> Sent: Friday, May 27, 2016 2:18 PM
> To: Jayathirth D V; Philip Race
> Cc: [hidden email]
> Subject: Re: Review Request for JDK-7107905: ColorModel subclasses are missing hashCode() or equals() or both methods
>
> Hi Jay,
>
> .equals() should not be comparing any fields that are computed from other fields - generally fields that come directly from a constructor argument tend to be the fields to compare.  Internal state variables (such as "isInited" and the like) should never be compared.  It should be comparing fields that affect the interpretation of the color.  To that end...
>
> In CM, add comparison/hash of:
>      ColorSpace
>      transferType
>
> In CCM, do not compare or hash:
>      needScaleInit (internal state for lazy ScaleInit)
>      noUnnorm (computed from other constructor arguments)
>      nonStdScale (computed from other constructor arguments)
>      signed (computed from other constructor arguments)
>      transferType (tested in CM)
> (basically, CCM doesn't have anything to compares as all of its significant values are compared in the super class)
>
> ...jim
>
> On 05/27/2016 01:02 AM, Jayathirth D V wrote:
>> Hi,
>>
>> Gentle reminder.
>> Please review updated fix:
>> http://cr.openjdk.java.net/~jdv/7107905/webrev.07/
>>
>> Thanks,
>> Jay
>>
>> -----Original Message-----
>> From: Jayathirth D V
>> Sent: Thursday, May 19, 2016 6:43 PM
>> To: Philip Race; Jim Graham
>> Cc: [hidden email]
>> Subject: Review Request for JDK-7107905: ColorModel subclasses are
>> missing hashCode() or equals() or both methods
>>
>> Hi,
>>
>> Previously for this bug we were making changes related only to IndexColorModel. Since we are expanding to include hashCode() or equals() method from PackedColorModel and ComponentColorModel, I have created single webrev for review under the same bug id.
>>
>> Now the "getclass()==" check is present in base class ColorModel and each subclass of ColorModel have their own equality check for properties.
>>
>> Details:
>> Bug : https://bugs.openjdk.java.net/browse/JDK-7107905
>> Updated Webrev : http://cr.openjdk.java.net/~jdv/7107905/webrev.07/
>>
>> Please review the changes at your convenience.
>>
>> Thanks,
>> Jay
>>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Review Request for JDK-7107905: ColorModel subclasses are missing hashCode() or equals() or both methods

Jayathirth D v
Hi Jim,

Thanks for your valuable inputs. I have made recommended changes.
Please find updated webrev for review:
http://cr.openjdk.java.net/~jdv/7107905/webrev.08/ 

Thanks,
Jay

-----Original Message-----
From: Jim Graham
Sent: Friday, May 27, 2016 11:52 PM
To: Jayathirth D V; Philip Race
Cc: [hidden email]
Subject: Re: Review Request for JDK-7107905: ColorModel subclasses are missing hashCode() or equals() or both methods

I think that makes sense.  If it has no type-specific information to compare, then it doesn't need to override them...

                        ...jim

On 05/27/2016 05:32 AM, Jayathirth D V wrote:

> Hi Jim,
>
> Since we will be moving comparison for ColorSpace and transferType to CM, can we remove equals() & hashCode() methods in CCM?
> For PCM and ICM we have unique variables which we can compare, there will be no problem.
>
> Thanks,
> Jay
>
> -----Original Message-----
> From: Jim Graham
> Sent: Friday, May 27, 2016 2:18 PM
> To: Jayathirth D V; Philip Race
> Cc: [hidden email]
> Subject: Re: Review Request for JDK-7107905: ColorModel subclasses are
> missing hashCode() or equals() or both methods
>
> Hi Jay,
>
> .equals() should not be comparing any fields that are computed from other fields - generally fields that come directly from a constructor argument tend to be the fields to compare.  Internal state variables (such as "isInited" and the like) should never be compared.  It should be comparing fields that affect the interpretation of the color.  To that end...
>
> In CM, add comparison/hash of:
>      ColorSpace
>      transferType
>
> In CCM, do not compare or hash:
>      needScaleInit (internal state for lazy ScaleInit)
>      noUnnorm (computed from other constructor arguments)
>      nonStdScale (computed from other constructor arguments)
>      signed (computed from other constructor arguments)
>      transferType (tested in CM)
> (basically, CCM doesn't have anything to compares as all of its
> significant values are compared in the super class)
>
> ...jim
>
> On 05/27/2016 01:02 AM, Jayathirth D V wrote:
>> Hi,
>>
>> Gentle reminder.
>> Please review updated fix:
>> http://cr.openjdk.java.net/~jdv/7107905/webrev.07/
>>
>> Thanks,
>> Jay
>>
>> -----Original Message-----
>> From: Jayathirth D V
>> Sent: Thursday, May 19, 2016 6:43 PM
>> To: Philip Race; Jim Graham
>> Cc: [hidden email]
>> Subject: Review Request for JDK-7107905: ColorModel subclasses are
>> missing hashCode() or equals() or both methods
>>
>> Hi,
>>
>> Previously for this bug we were making changes related only to IndexColorModel. Since we are expanding to include hashCode() or equals() method from PackedColorModel and ComponentColorModel, I have created single webrev for review under the same bug id.
>>
>> Now the "getclass()==" check is present in base class ColorModel and each subclass of ColorModel have their own equality check for properties.
>>
>> Details:
>> Bug : https://bugs.openjdk.java.net/browse/JDK-7107905
>> Updated Webrev : http://cr.openjdk.java.net/~jdv/7107905/webrev.07/
>>
>> Please review the changes at your convenience.
>>
>> Thanks,
>> Jay
>>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Review Request for JDK-7107905: ColorModel subclasses are missing hashCode() or equals() or both methods

Philip Race

1449      * the target object must be the exact same class as this

I believe the language people like to say "the same type" so maybe say

1449      * the target object must be of the same type (i.e. class) as this

Could IndexColorModel cache the hash code since it could be fairly expensive to compute ?
Arrays.hashCode() is the expensive bit.

Less so PackedColorModel but could be considered there too.

No other comments at this time.

-phil.

On 05/29/2016 11:55 PM, Jayathirth D V wrote:

> Hi Jim,
>
> Thanks for your valuable inputs. I have made recommended changes.
> Please find updated webrev for review:
> http://cr.openjdk.java.net/~jdv/7107905/webrev.08/
>
> Thanks,
> Jay
>
> -----Original Message-----
> From: Jim Graham
> Sent: Friday, May 27, 2016 11:52 PM
> To: Jayathirth D V; Philip Race
> Cc: [hidden email]
> Subject: Re: Review Request for JDK-7107905: ColorModel subclasses are missing hashCode() or equals() or both methods
>
> I think that makes sense.  If it has no type-specific information to compare, then it doesn't need to override them...
>
> ...jim
>
> On 05/27/2016 05:32 AM, Jayathirth D V wrote:
>> Hi Jim,
>>
>> Since we will be moving comparison for ColorSpace and transferType to CM, can we remove equals() & hashCode() methods in CCM?
>> For PCM and ICM we have unique variables which we can compare, there will be no problem.
>>
>> Thanks,
>> Jay
>>
>> -----Original Message-----
>> From: Jim Graham
>> Sent: Friday, May 27, 2016 2:18 PM
>> To: Jayathirth D V; Philip Race
>> Cc: [hidden email]
>> Subject: Re: Review Request for JDK-7107905: ColorModel subclasses are
>> missing hashCode() or equals() or both methods
>>
>> Hi Jay,
>>
>> .equals() should not be comparing any fields that are computed from other fields - generally fields that come directly from a constructor argument tend to be the fields to compare.  Internal state variables (such as "isInited" and the like) should never be compared.  It should be comparing fields that affect the interpretation of the color.  To that end...
>>
>> In CM, add comparison/hash of:
>>       ColorSpace
>>       transferType
>>
>> In CCM, do not compare or hash:
>>       needScaleInit (internal state for lazy ScaleInit)
>>       noUnnorm (computed from other constructor arguments)
>>       nonStdScale (computed from other constructor arguments)
>>       signed (computed from other constructor arguments)
>>       transferType (tested in CM)
>> (basically, CCM doesn't have anything to compares as all of its
>> significant values are compared in the super class)
>>
>> ...jim
>>
>> On 05/27/2016 01:02 AM, Jayathirth D V wrote:
>>> Hi,
>>>
>>> Gentle reminder.
>>> Please review updated fix:
>>> http://cr.openjdk.java.net/~jdv/7107905/webrev.07/
>>>
>>> Thanks,
>>> Jay
>>>
>>> -----Original Message-----
>>> From: Jayathirth D V
>>> Sent: Thursday, May 19, 2016 6:43 PM
>>> To: Philip Race; Jim Graham
>>> Cc: [hidden email]
>>> Subject: Review Request for JDK-7107905: ColorModel subclasses are
>>> missing hashCode() or equals() or both methods
>>>
>>> Hi,
>>>
>>> Previously for this bug we were making changes related only to IndexColorModel. Since we are expanding to include hashCode() or equals() method from PackedColorModel and ComponentColorModel, I have created single webrev for review under the same bug id.
>>>
>>> Now the "getclass()==" check is present in base class ColorModel and each subclass of ColorModel have their own equality check for properties.
>>>
>>> Details:
>>> Bug : https://bugs.openjdk.java.net/browse/JDK-7107905
>>> Updated Webrev : http://cr.openjdk.java.net/~jdv/7107905/webrev.07/
>>>
>>> Please review the changes at your convenience.
>>>
>>> Thanks,
>>> Jay
>>>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Review Request for JDK-7107905: ColorModel subclasses are missing hashCode() or equals() or both methods

Jim Graham-5
Hi Phil,

I don't think that alternate wording conveys that this is not a case of
"instanceof" and is instead a case of "getClass() =="...

Perhaps we don't need "exact", but "type" is vague enough to call into
question how we do the test.  I could see "the same class (and not a
subclass) as this"...?

                        ...jim

On 05/31/2016 11:56 AM, Phil Race wrote:

>
> 1449      * the target object must be the exact same class as this
>
> I believe the language people like to say "the same type" so maybe say
>
> 1449      * the target object must be of the same type (i.e. class) as this
>
> Could IndexColorModel cache the hash code since it could be fairly
> expensive to compute ?
> Arrays.hashCode() is the expensive bit.
>
> Less so PackedColorModel but could be considered there too.
>
> No other comments at this time.
>
> -phil.
>
> On 05/29/2016 11:55 PM, Jayathirth D V wrote:
>> Hi Jim,
>>
>> Thanks for your valuable inputs. I have made recommended changes.
>> Please find updated webrev for review:
>> http://cr.openjdk.java.net/~jdv/7107905/webrev.08/
>>
>> Thanks,
>> Jay
>>
>> -----Original Message-----
>> From: Jim Graham
>> Sent: Friday, May 27, 2016 11:52 PM
>> To: Jayathirth D V; Philip Race
>> Cc: [hidden email]
>> Subject: Re: Review Request for JDK-7107905: ColorModel subclasses are
>> missing hashCode() or equals() or both methods
>>
>> I think that makes sense.  If it has no type-specific information to
>> compare, then it doesn't need to override them...
>>
>>             ...jim
>>
>> On 05/27/2016 05:32 AM, Jayathirth D V wrote:
>>> Hi Jim,
>>>
>>> Since we will be moving comparison for ColorSpace and transferType to
>>> CM, can we remove equals() & hashCode() methods in CCM?
>>> For PCM and ICM we have unique variables which we can compare, there
>>> will be no problem.
>>>
>>> Thanks,
>>> Jay
>>>
>>> -----Original Message-----
>>> From: Jim Graham
>>> Sent: Friday, May 27, 2016 2:18 PM
>>> To: Jayathirth D V; Philip Race
>>> Cc: [hidden email]
>>> Subject: Re: Review Request for JDK-7107905: ColorModel subclasses are
>>> missing hashCode() or equals() or both methods
>>>
>>> Hi Jay,
>>>
>>> .equals() should not be comparing any fields that are computed from
>>> other fields - generally fields that come directly from a constructor
>>> argument tend to be the fields to compare.  Internal state variables
>>> (such as "isInited" and the like) should never be compared.  It
>>> should be comparing fields that affect the interpretation of the
>>> color.  To that end...
>>>
>>> In CM, add comparison/hash of:
>>>       ColorSpace
>>>       transferType
>>>
>>> In CCM, do not compare or hash:
>>>       needScaleInit (internal state for lazy ScaleInit)
>>>       noUnnorm (computed from other constructor arguments)
>>>       nonStdScale (computed from other constructor arguments)
>>>       signed (computed from other constructor arguments)
>>>       transferType (tested in CM)
>>> (basically, CCM doesn't have anything to compares as all of its
>>> significant values are compared in the super class)
>>>
>>>             ...jim
>>>
>>> On 05/27/2016 01:02 AM, Jayathirth D V wrote:
>>>> Hi,
>>>>
>>>> Gentle reminder.
>>>> Please review updated fix:
>>>> http://cr.openjdk.java.net/~jdv/7107905/webrev.07/
>>>>
>>>> Thanks,
>>>> Jay
>>>>
>>>> -----Original Message-----
>>>> From: Jayathirth D V
>>>> Sent: Thursday, May 19, 2016 6:43 PM
>>>> To: Philip Race; Jim Graham
>>>> Cc: [hidden email]
>>>> Subject: Review Request for JDK-7107905: ColorModel subclasses are
>>>> missing hashCode() or equals() or both methods
>>>>
>>>> Hi,
>>>>
>>>> Previously for this bug we were making changes related only to
>>>> IndexColorModel. Since we are expanding to include hashCode() or
>>>> equals() method from PackedColorModel and ComponentColorModel, I
>>>> have created single webrev for review under the same bug id.
>>>>
>>>> Now the "getclass()==" check is present in base class ColorModel and
>>>> each subclass of ColorModel have their own equality check for
>>>> properties.
>>>>
>>>> Details:
>>>> Bug : https://bugs.openjdk.java.net/browse/JDK-7107905
>>>> Updated Webrev : http://cr.openjdk.java.net/~jdv/7107905/webrev.07/
>>>>
>>>> Please review the changes at your convenience.
>>>>
>>>> Thanks,
>>>> Jay
>>>>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Review Request for JDK-7107905: ColorModel subclasses are missing hashCode() or equals() or both methods

Jim Graham-5
In reply to this post by Jayathirth D v
Hi Jay,

You were going to remove hashCode/equals from CCM, but instead you
simply removed it from the patch.  You still need to edit it to remove
the existing methods.

Also, I'm not sure == is a good way to compare ColorSpace instances - Phil?

                        ...jim

On 05/29/2016 11:55 PM, Jayathirth D V wrote:

> Hi Jim,
>
> Thanks for your valuable inputs. I have made recommended changes.
> Please find updated webrev for review:
> http://cr.openjdk.java.net/~jdv/7107905/webrev.08/
>
> Thanks,
> Jay
>
> -----Original Message-----
> From: Jim Graham
> Sent: Friday, May 27, 2016 11:52 PM
> To: Jayathirth D V; Philip Race
> Cc: [hidden email]
> Subject: Re: Review Request for JDK-7107905: ColorModel subclasses are missing hashCode() or equals() or both methods
>
> I think that makes sense.  If it has no type-specific information to compare, then it doesn't need to override them...
>
> ...jim
>
> On 05/27/2016 05:32 AM, Jayathirth D V wrote:
>> Hi Jim,
>>
>> Since we will be moving comparison for ColorSpace and transferType to CM, can we remove equals() & hashCode() methods in CCM?
>> For PCM and ICM we have unique variables which we can compare, there will be no problem.
>>
>> Thanks,
>> Jay
>>
>> -----Original Message-----
>> From: Jim Graham
>> Sent: Friday, May 27, 2016 2:18 PM
>> To: Jayathirth D V; Philip Race
>> Cc: [hidden email]
>> Subject: Re: Review Request for JDK-7107905: ColorModel subclasses are
>> missing hashCode() or equals() or both methods
>>
>> Hi Jay,
>>
>> .equals() should not be comparing any fields that are computed from other fields - generally fields that come directly from a constructor argument tend to be the fields to compare.  Internal state variables (such as "isInited" and the like) should never be compared.  It should be comparing fields that affect the interpretation of the color.  To that end...
>>
>> In CM, add comparison/hash of:
>>       ColorSpace
>>       transferType
>>
>> In CCM, do not compare or hash:
>>       needScaleInit (internal state for lazy ScaleInit)
>>       noUnnorm (computed from other constructor arguments)
>>       nonStdScale (computed from other constructor arguments)
>>       signed (computed from other constructor arguments)
>>       transferType (tested in CM)
>> (basically, CCM doesn't have anything to compares as all of its
>> significant values are compared in the super class)
>>
>> ...jim
>>
>> On 05/27/2016 01:02 AM, Jayathirth D V wrote:
>>> Hi,
>>>
>>> Gentle reminder.
>>> Please review updated fix:
>>> http://cr.openjdk.java.net/~jdv/7107905/webrev.07/
>>>
>>> Thanks,
>>> Jay
>>>
>>> -----Original Message-----
>>> From: Jayathirth D V
>>> Sent: Thursday, May 19, 2016 6:43 PM
>>> To: Philip Race; Jim Graham
>>> Cc: [hidden email]
>>> Subject: Review Request for JDK-7107905: ColorModel subclasses are
>>> missing hashCode() or equals() or both methods
>>>
>>> Hi,
>>>
>>> Previously for this bug we were making changes related only to IndexColorModel. Since we are expanding to include hashCode() or equals() method from PackedColorModel and ComponentColorModel, I have created single webrev for review under the same bug id.
>>>
>>> Now the "getclass()==" check is present in base class ColorModel and each subclass of ColorModel have their own equality check for properties.
>>>
>>> Details:
>>> Bug : https://bugs.openjdk.java.net/browse/JDK-7107905
>>> Updated Webrev : http://cr.openjdk.java.net/~jdv/7107905/webrev.07/
>>>
>>> Please review the changes at your convenience.
>>>
>>> Thanks,
>>> Jay
>>>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Review Request for JDK-7107905: ColorModel subclasses are missing hashCode() or equals() or both methods

Philip Race
In reply to this post by Jim Graham-5
On 05/31/2016 12:16 PM, Jim Graham wrote:
> Hi Phil,
>
> I don't think that alternate wording conveys that this is not a case
> of "instanceof" and is instead a case of "getClass() =="...
>
> Perhaps we don't need "exact", but "type" is vague enough to call into
> question how we do the test.  I could see "the same class (and not a
> subclass) as this"...?

That wording works for me. I just thought someone else might prefer
'type' strongly enough to ask for it.

-phil.

>
>             ...jim
>
> On 05/31/2016 11:56 AM, Phil Race wrote:
>>
>> 1449      * the target object must be the exact same class as this
>>
>> I believe the language people like to say "the same type" so maybe say
>>
>> 1449      * the target object must be of the same type (i.e. class)
>> as this
>>
>> Could IndexColorModel cache the hash code since it could be fairly
>> expensive to compute ?
>> Arrays.hashCode() is the expensive bit.
>>
>> Less so PackedColorModel but could be considered there too.
>>
>> No other comments at this time.
>>
>> -phil.
>>
>> On 05/29/2016 11:55 PM, Jayathirth D V wrote:
>>> Hi Jim,
>>>
>>> Thanks for your valuable inputs. I have made recommended changes.
>>> Please find updated webrev for review:
>>> http://cr.openjdk.java.net/~jdv/7107905/webrev.08/
>>>
>>> Thanks,
>>> Jay
>>>
>>> -----Original Message-----
>>> From: Jim Graham
>>> Sent: Friday, May 27, 2016 11:52 PM
>>> To: Jayathirth D V; Philip Race
>>> Cc: [hidden email]
>>> Subject: Re: Review Request for JDK-7107905: ColorModel subclasses are
>>> missing hashCode() or equals() or both methods
>>>
>>> I think that makes sense.  If it has no type-specific information to
>>> compare, then it doesn't need to override them...
>>>
>>>             ...jim
>>>
>>> On 05/27/2016 05:32 AM, Jayathirth D V wrote:
>>>> Hi Jim,
>>>>
>>>> Since we will be moving comparison for ColorSpace and transferType to
>>>> CM, can we remove equals() & hashCode() methods in CCM?
>>>> For PCM and ICM we have unique variables which we can compare, there
>>>> will be no problem.
>>>>
>>>> Thanks,
>>>> Jay
>>>>
>>>> -----Original Message-----
>>>> From: Jim Graham
>>>> Sent: Friday, May 27, 2016 2:18 PM
>>>> To: Jayathirth D V; Philip Race
>>>> Cc: [hidden email]
>>>> Subject: Re: Review Request for JDK-7107905: ColorModel subclasses are
>>>> missing hashCode() or equals() or both methods
>>>>
>>>> Hi Jay,
>>>>
>>>> .equals() should not be comparing any fields that are computed from
>>>> other fields - generally fields that come directly from a constructor
>>>> argument tend to be the fields to compare.  Internal state variables
>>>> (such as "isInited" and the like) should never be compared. It
>>>> should be comparing fields that affect the interpretation of the
>>>> color.  To that end...
>>>>
>>>> In CM, add comparison/hash of:
>>>>       ColorSpace
>>>>       transferType
>>>>
>>>> In CCM, do not compare or hash:
>>>>       needScaleInit (internal state for lazy ScaleInit)
>>>>       noUnnorm (computed from other constructor arguments)
>>>>       nonStdScale (computed from other constructor arguments)
>>>>       signed (computed from other constructor arguments)
>>>>       transferType (tested in CM)
>>>> (basically, CCM doesn't have anything to compares as all of its
>>>> significant values are compared in the super class)
>>>>
>>>>             ...jim
>>>>
>>>> On 05/27/2016 01:02 AM, Jayathirth D V wrote:
>>>>> Hi,
>>>>>
>>>>> Gentle reminder.
>>>>> Please review updated fix:
>>>>> http://cr.openjdk.java.net/~jdv/7107905/webrev.07/
>>>>>
>>>>> Thanks,
>>>>> Jay
>>>>>
>>>>> -----Original Message-----
>>>>> From: Jayathirth D V
>>>>> Sent: Thursday, May 19, 2016 6:43 PM
>>>>> To: Philip Race; Jim Graham
>>>>> Cc: [hidden email]
>>>>> Subject: Review Request for JDK-7107905: ColorModel subclasses are
>>>>> missing hashCode() or equals() or both methods
>>>>>
>>>>> Hi,
>>>>>
>>>>> Previously for this bug we were making changes related only to
>>>>> IndexColorModel. Since we are expanding to include hashCode() or
>>>>> equals() method from PackedColorModel and ComponentColorModel, I
>>>>> have created single webrev for review under the same bug id.
>>>>>
>>>>> Now the "getclass()==" check is present in base class ColorModel and
>>>>> each subclass of ColorModel have their own equality check for
>>>>> properties.
>>>>>
>>>>> Details:
>>>>> Bug : https://bugs.openjdk.java.net/browse/JDK-7107905
>>>>> Updated Webrev : http://cr.openjdk.java.net/~jdv/7107905/webrev.07/
>>>>>
>>>>> Please review the changes at your convenience.
>>>>>
>>>>> Thanks,
>>>>> Jay
>>>>>
>>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Review Request for JDK-7107905: ColorModel subclasses are missing hashCode() or equals() or both methods

Philip Race
In reply to this post by Jim Graham-5
On 05/31/2016 12:20 PM, Jim Graham wrote:
> Hi Jay,
>
> You were going to remove hashCode/equals from CCM, but instead you
> simply removed it from the patch.  You still need to edit it to remove
> the existing methods.

Oh yeah ! CCM is gone from the latest webrev. I expect that was not
intentional.

>
> Also, I'm not sure == is a good way to compare ColorSpace instances -
> Phil?

Neither ColorSpace nor ICC_ColorSpace over-ride equals or hashCode and
all the predefined ones are constructed as singletons so it seems unlikely
to cause problems

-phil.

>
>             ...jim
>
> On 05/29/2016 11:55 PM, Jayathirth D V wrote:
>> Hi Jim,
>>
>> Thanks for your valuable inputs. I have made recommended changes.
>> Please find updated webrev for review:
>> http://cr.openjdk.java.net/~jdv/7107905/webrev.08/
>>
>> Thanks,
>> Jay
>>
>> -----Original Message-----
>> From: Jim Graham
>> Sent: Friday, May 27, 2016 11:52 PM
>> To: Jayathirth D V; Philip Race
>> Cc: [hidden email]
>> Subject: Re: Review Request for JDK-7107905: ColorModel subclasses
>> are missing hashCode() or equals() or both methods
>>
>> I think that makes sense.  If it has no type-specific information to
>> compare, then it doesn't need to override them...
>>
>>             ...jim
>>
>> On 05/27/2016 05:32 AM, Jayathirth D V wrote:
>>> Hi Jim,
>>>
>>> Since we will be moving comparison for ColorSpace and transferType
>>> to CM, can we remove equals() & hashCode() methods in CCM?
>>> For PCM and ICM we have unique variables which we can compare, there
>>> will be no problem.
>>>
>>> Thanks,
>>> Jay
>>>
>>> -----Original Message-----
>>> From: Jim Graham
>>> Sent: Friday, May 27, 2016 2:18 PM
>>> To: Jayathirth D V; Philip Race
>>> Cc: [hidden email]
>>> Subject: Re: Review Request for JDK-7107905: ColorModel subclasses are
>>> missing hashCode() or equals() or both methods
>>>
>>> Hi Jay,
>>>
>>> .equals() should not be comparing any fields that are computed from
>>> other fields - generally fields that come directly from a
>>> constructor argument tend to be the fields to compare. Internal
>>> state variables (such as "isInited" and the like) should never be
>>> compared.  It should be comparing fields that affect the
>>> interpretation of the color.  To that end...
>>>
>>> In CM, add comparison/hash of:
>>>       ColorSpace
>>>       transferType
>>>
>>> In CCM, do not compare or hash:
>>>       needScaleInit (internal state for lazy ScaleInit)
>>>       noUnnorm (computed from other constructor arguments)
>>>       nonStdScale (computed from other constructor arguments)
>>>       signed (computed from other constructor arguments)
>>>       transferType (tested in CM)
>>> (basically, CCM doesn't have anything to compares as all of its
>>> significant values are compared in the super class)
>>>
>>>             ...jim
>>>
>>> On 05/27/2016 01:02 AM, Jayathirth D V wrote:
>>>> Hi,
>>>>
>>>> Gentle reminder.
>>>> Please review updated fix:
>>>> http://cr.openjdk.java.net/~jdv/7107905/webrev.07/
>>>>
>>>> Thanks,
>>>> Jay
>>>>
>>>> -----Original Message-----
>>>> From: Jayathirth D V
>>>> Sent: Thursday, May 19, 2016 6:43 PM
>>>> To: Philip Race; Jim Graham
>>>> Cc: [hidden email]
>>>> Subject: Review Request for JDK-7107905: ColorModel subclasses are
>>>> missing hashCode() or equals() or both methods
>>>>
>>>> Hi,
>>>>
>>>> Previously for this bug we were making changes related only to
>>>> IndexColorModel. Since we are expanding to include hashCode() or
>>>> equals() method from PackedColorModel and ComponentColorModel, I
>>>> have created single webrev for review under the same bug id.
>>>>
>>>> Now the "getclass()==" check is present in base class ColorModel
>>>> and each subclass of ColorModel have their own equality check for
>>>> properties.
>>>>
>>>> Details:
>>>> Bug : https://bugs.openjdk.java.net/browse/JDK-7107905
>>>> Updated Webrev : http://cr.openjdk.java.net/~jdv/7107905/webrev.07/
>>>>
>>>> Please review the changes at your convenience.
>>>>
>>>> Thanks,
>>>> Jay
>>>>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Review Request for JDK-7107905: ColorModel subclasses are missing hashCode() or equals() or both methods

Jim Graham-5


On 05/31/2016 02:50 PM, Phil Race wrote:

> On 05/31/2016 12:20 PM, Jim Graham wrote:
>> Hi Jay,
>>
>> You were going to remove hashCode/equals from CCM, but instead you
>> simply removed it from the patch.  You still need to edit it to remove
>> the existing methods.
>
> Oh yeah ! CCM is gone from the latest webrev. I expect that was not
> intentional.
>
>>
>> Also, I'm not sure == is a good way to compare ColorSpace instances -
>> Phil?
>
> Neither ColorSpace nor ICC_ColorSpace over-ride equals or hashCode and
> all the predefined ones are constructed as singletons so it seems unlikely
> to cause problems

Should it use .equals() on principle, though?  Otherwise we are baking
in the assumption that it doesn't implement equals() into our
implementation of the CM.equals() method.  Might it some day implement
equals (perhaps it isn't a practical issue today, but it might be in the
future when we forget that it was omitted in this usage we create today).

I guess what I'm asking is if it is a design feature that different
objects are considered non-equal (such as an object that, for instance,
has only predetermined instances that are shared by reference and
reused).  I think color spaces are sort of in-between in that we define
a few constants that simply get used by reference in 99% of cases, but
that isn't a design feature, more like a practical issue...

                        ...jim
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Review Request for JDK-7107905: ColorModel subclasses are missing hashCode() or equals() or both methods

Philip Race
I don't know of any design intent, so yes, I meant more as a practical
matter.
Unless they subclass then using equals() which I thought unlikely it
makes no difference here.
But it would be proof against that to use equals, unlikely to matter as
it might be ..

-phil.

On 05/31/2016 03:19 PM, Jim Graham wrote:

>
>
> On 05/31/2016 02:50 PM, Phil Race wrote:
>> On 05/31/2016 12:20 PM, Jim Graham wrote:
>>> Hi Jay,
>>>
>>> You were going to remove hashCode/equals from CCM, but instead you
>>> simply removed it from the patch.  You still need to edit it to remove
>>> the existing methods.
>>
>> Oh yeah ! CCM is gone from the latest webrev. I expect that was not
>> intentional.
>>
>>>
>>> Also, I'm not sure == is a good way to compare ColorSpace instances -
>>> Phil?
>>
>> Neither ColorSpace nor ICC_ColorSpace over-ride equals or hashCode and
>> all the predefined ones are constructed as singletons so it seems
>> unlikely
>> to cause problems
>
> Should it use .equals() on principle, though?  Otherwise we are baking
> in the assumption that it doesn't implement equals() into our
> implementation of the CM.equals() method.  Might it some day implement
> equals (perhaps it isn't a practical issue today, but it might be in
> the future when we forget that it was omitted in this usage we create
> today).
>
> I guess what I'm asking is if it is a design feature that different
> objects are considered non-equal (such as an object that, for
> instance, has only predetermined instances that are shared by
> reference and reused).  I think color spaces are sort of in-between in
> that we define a few constants that simply get used by reference in
> 99% of cases, but that isn't a design feature, more like a practical
> issue...
>
>             ...jim

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Review Request for JDK-7107905: ColorModel subclasses are missing hashCode() or equals() or both methods

Jim Graham-5
I think we should use .equals() here, otherwise it looks like a
recommendation that the intent is for those classes to never implement it...

                        ...jim

On 05/31/2016 03:31 PM, Phil Race wrote:

> I don't know of any design intent, so yes, I meant more as a practical
> matter.
> Unless they subclass then using equals() which I thought unlikely it
> makes no difference here.
> But it would be proof against that to use equals, unlikely to matter as
> it might be ..
>
> -phil.
>
> On 05/31/2016 03:19 PM, Jim Graham wrote:
>>
>>
>> On 05/31/2016 02:50 PM, Phil Race wrote:
>>> On 05/31/2016 12:20 PM, Jim Graham wrote:
>>>> Hi Jay,
>>>>
>>>> You were going to remove hashCode/equals from CCM, but instead you
>>>> simply removed it from the patch.  You still need to edit it to remove
>>>> the existing methods.
>>>
>>> Oh yeah ! CCM is gone from the latest webrev. I expect that was not
>>> intentional.
>>>
>>>>
>>>> Also, I'm not sure == is a good way to compare ColorSpace instances -
>>>> Phil?
>>>
>>> Neither ColorSpace nor ICC_ColorSpace over-ride equals or hashCode and
>>> all the predefined ones are constructed as singletons so it seems
>>> unlikely
>>> to cause problems
>>
>> Should it use .equals() on principle, though?  Otherwise we are baking
>> in the assumption that it doesn't implement equals() into our
>> implementation of the CM.equals() method.  Might it some day implement
>> equals (perhaps it isn't a practical issue today, but it might be in
>> the future when we forget that it was omitted in this usage we create
>> today).
>>
>> I guess what I'm asking is if it is a design feature that different
>> objects are considered non-equal (such as an object that, for
>> instance, has only predetermined instances that are shared by
>> reference and reused).  I think color spaces are sort of in-between in
>> that we define a few constants that simply get used by reference in
>> 99% of cases, but that isn't a design feature, more like a practical
>> issue...
>>
>>             ...jim
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Review Request for JDK-7107905: ColorModel subclasses are missing hashCode() or equals() or both methods

Jayathirth D v
Hi Phil & Jim,

Collating all the changes needed:
1) I have removed both equals() & hashCode() in CCM but forgot to add the file while creating webrev. I will include changes in CCM in updated webrev.
2) Caching of hashCode value in IndexColorModel & PackedColorModel :
        We can cache hashCode value when constructor is called or when hashCode() is called for first time. What approach we have to follow?
3) Comment section of equals() method, I will update it to :
        1449      * the target object must be the same class (and not a subclass) as this
4) I will use .equals() to compare colorSpace values in CM.equals() so it will be like we are not intending ColorSpace class to never override equals() method.

Please let me know your inputs.

Thanks,
Jay

-----Original Message-----
From: Jim Graham
Sent: Wednesday, June 01, 2016 4:14 AM
To: Phil Race
Cc: Jayathirth D V; [hidden email]
Subject: Re: Review Request for JDK-7107905: ColorModel subclasses are missing hashCode() or equals() or both methods

I think we should use .equals() here, otherwise it looks like a recommendation that the intent is for those classes to never implement it...

                        ...jim

On 05/31/2016 03:31 PM, Phil Race wrote:

> I don't know of any design intent, so yes, I meant more as a practical
> matter.
> Unless they subclass then using equals() which I thought unlikely it
> makes no difference here.
> But it would be proof against that to use equals, unlikely to matter
> as it might be ..
>
> -phil.
>
> On 05/31/2016 03:19 PM, Jim Graham wrote:
>>
>>
>> On 05/31/2016 02:50 PM, Phil Race wrote:
>>> On 05/31/2016 12:20 PM, Jim Graham wrote:
>>>> Hi Jay,
>>>>
>>>> You were going to remove hashCode/equals from CCM, but instead you
>>>> simply removed it from the patch.  You still need to edit it to
>>>> remove the existing methods.
>>>
>>> Oh yeah ! CCM is gone from the latest webrev. I expect that was not
>>> intentional.
>>>
>>>>
>>>> Also, I'm not sure == is a good way to compare ColorSpace instances
>>>> - Phil?
>>>
>>> Neither ColorSpace nor ICC_ColorSpace over-ride equals or hashCode
>>> and all the predefined ones are constructed as singletons so it
>>> seems unlikely to cause problems
>>
>> Should it use .equals() on principle, though?  Otherwise we are
>> baking in the assumption that it doesn't implement equals() into our
>> implementation of the CM.equals() method.  Might it some day
>> implement equals (perhaps it isn't a practical issue today, but it
>> might be in the future when we forget that it was omitted in this
>> usage we create today).
>>
>> I guess what I'm asking is if it is a design feature that different
>> objects are considered non-equal (such as an object that, for
>> instance, has only predetermined instances that are shared by
>> reference and reused).  I think color spaces are sort of in-between
>> in that we define a few constants that simply get used by reference
>> in 99% of cases, but that isn't a design feature, more like a
>> practical issue...
>>
>>             ...jim
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Review Request for JDK-7107905: ColorModel subclasses are missing hashCode() or equals() or both methods

Philip Race
Please post the updated webrev.

-phil.

On 6/1/16, 12:02 AM, Jayathirth D V wrote:

> Hi Phil&  Jim,
>
> Collating all the changes needed:
> 1) I have removed both equals()&  hashCode() in CCM but forgot to add the file while creating webrev. I will include changes in CCM in updated webrev.
> 2) Caching of hashCode value in IndexColorModel&  PackedColorModel :
> We can cache hashCode value when constructor is called or when hashCode() is called for first time. What approach we have to follow?
> 3) Comment section of equals() method, I will update it to :
> 1449      * the target object must be the same class (and not a subclass) as this
> 4) I will use .equals() to compare colorSpace values in CM.equals() so it will be like we are not intending ColorSpace class to never override equals() method.
>
> Please let me know your inputs.
>
> Thanks,
> Jay
>
> -----Original Message-----
> From: Jim Graham
> Sent: Wednesday, June 01, 2016 4:14 AM
> To: Phil Race
> Cc: Jayathirth D V; [hidden email]
> Subject: Re: Review Request for JDK-7107905: ColorModel subclasses are missing hashCode() or equals() or both methods
>
> I think we should use .equals() here, otherwise it looks like a recommendation that the intent is for those classes to never implement it...
>
> ...jim
>
> On 05/31/2016 03:31 PM, Phil Race wrote:
>> I don't know of any design intent, so yes, I meant more as a practical
>> matter.
>> Unless they subclass then using equals() which I thought unlikely it
>> makes no difference here.
>> But it would be proof against that to use equals, unlikely to matter
>> as it might be ..
>>
>> -phil.
>>
>> On 05/31/2016 03:19 PM, Jim Graham wrote:
>>>
>>> On 05/31/2016 02:50 PM, Phil Race wrote:
>>>> On 05/31/2016 12:20 PM, Jim Graham wrote:
>>>>> Hi Jay,
>>>>>
>>>>> You were going to remove hashCode/equals from CCM, but instead you
>>>>> simply removed it from the patch.  You still need to edit it to
>>>>> remove the existing methods.
>>>> Oh yeah ! CCM is gone from the latest webrev. I expect that was not
>>>> intentional.
>>>>
>>>>> Also, I'm not sure == is a good way to compare ColorSpace instances
>>>>> - Phil?
>>>> Neither ColorSpace nor ICC_ColorSpace over-ride equals or hashCode
>>>> and all the predefined ones are constructed as singletons so it
>>>> seems unlikely to cause problems
>>> Should it use .equals() on principle, though?  Otherwise we are
>>> baking in the assumption that it doesn't implement equals() into our
>>> implementation of the CM.equals() method.  Might it some day
>>> implement equals (perhaps it isn't a practical issue today, but it
>>> might be in the future when we forget that it was omitted in this
>>> usage we create today).
>>>
>>> I guess what I'm asking is if it is a design feature that different
>>> objects are considered non-equal (such as an object that, for
>>> instance, has only predetermined instances that are shared by
>>> reference and reused).  I think color spaces are sort of in-between
>>> in that we define a few constants that simply get used by reference
>>> in 99% of cases, but that isn't a design feature, more like a
>>> practical issue...
>>>
>>>              ...jim
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Review Request for JDK-7107905: ColorModel subclasses are missing hashCode() or equals() or both methods

Jayathirth D v
Hi Phil,

I have updated the code with all the changes I mentioned previously. I am caching hashCode when first time hashCode() is called.
Please find the updated webrev for review:
http://cr.openjdk.java.net/~jdv/7107905/webrev.09/ 

Thanks,
Jay

-----Original Message-----
From: Philip Race
Sent: Wednesday, June 01, 2016 10:06 PM
To: Jayathirth D V
Cc: Jim Graham; [hidden email]
Subject: Re: Review Request for JDK-7107905: ColorModel subclasses are missing hashCode() or equals() or both methods

Please post the updated webrev.

-phil.

On 6/1/16, 12:02 AM, Jayathirth D V wrote:

> Hi Phil&  Jim,
>
> Collating all the changes needed:
> 1) I have removed both equals()&  hashCode() in CCM but forgot to add the file while creating webrev. I will include changes in CCM in updated webrev.
> 2) Caching of hashCode value in IndexColorModel&  PackedColorModel :
> We can cache hashCode value when constructor is called or when hashCode() is called for first time. What approach we have to follow?
> 3) Comment section of equals() method, I will update it to :
> 1449      * the target object must be the same class (and not a subclass) as this
> 4) I will use .equals() to compare colorSpace values in CM.equals() so it will be like we are not intending ColorSpace class to never override equals() method.
>
> Please let me know your inputs.
>
> Thanks,
> Jay
>
> -----Original Message-----
> From: Jim Graham
> Sent: Wednesday, June 01, 2016 4:14 AM
> To: Phil Race
> Cc: Jayathirth D V; [hidden email]
> Subject: Re: Review Request for JDK-7107905: ColorModel subclasses are
> missing hashCode() or equals() or both methods
>
> I think we should use .equals() here, otherwise it looks like a recommendation that the intent is for those classes to never implement it...
>
> ...jim
>
> On 05/31/2016 03:31 PM, Phil Race wrote:
>> I don't know of any design intent, so yes, I meant more as a
>> practical matter.
>> Unless they subclass then using equals() which I thought unlikely it
>> makes no difference here.
>> But it would be proof against that to use equals, unlikely to matter
>> as it might be ..
>>
>> -phil.
>>
>> On 05/31/2016 03:19 PM, Jim Graham wrote:
>>>
>>> On 05/31/2016 02:50 PM, Phil Race wrote:
>>>> On 05/31/2016 12:20 PM, Jim Graham wrote:
>>>>> Hi Jay,
>>>>>
>>>>> You were going to remove hashCode/equals from CCM, but instead you
>>>>> simply removed it from the patch.  You still need to edit it to
>>>>> remove the existing methods.
>>>> Oh yeah ! CCM is gone from the latest webrev. I expect that was not
>>>> intentional.
>>>>
>>>>> Also, I'm not sure == is a good way to compare ColorSpace
>>>>> instances
>>>>> - Phil?
>>>> Neither ColorSpace nor ICC_ColorSpace over-ride equals or hashCode
>>>> and all the predefined ones are constructed as singletons so it
>>>> seems unlikely to cause problems
>>> Should it use .equals() on principle, though?  Otherwise we are
>>> baking in the assumption that it doesn't implement equals() into our
>>> implementation of the CM.equals() method.  Might it some day
>>> implement equals (perhaps it isn't a practical issue today, but it
>>> might be in the future when we forget that it was omitted in this
>>> usage we create today).
>>>
>>> I guess what I'm asking is if it is a design feature that different
>>> objects are considered non-equal (such as an object that, for
>>> instance, has only predetermined instances that are shared by
>>> reference and reused).  I think color spaces are sort of in-between
>>> in that we define a few constants that simply get used by reference
>>> in 99% of cases, but that isn't a design feature, more like a
>>> practical issue...
>>>
>>>              ...jim
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Review Request for JDK-7107905: ColorModel subclasses are missing hashCode() or equals() or both methods

Philip Race

+1

-phil.

On 06/02/2016 07:07 AM, Jayathirth D V wrote:

> Hi Phil,
>
> I have updated the code with all the changes I mentioned previously. I am caching hashCode when first time hashCode() is called.
> Please find the updated webrev for review:
> http://cr.openjdk.java.net/~jdv/7107905/webrev.09/
>
> Thanks,
> Jay
>
> -----Original Message-----
> From: Philip Race
> Sent: Wednesday, June 01, 2016 10:06 PM
> To: Jayathirth D V
> Cc: Jim Graham; [hidden email]
> Subject: Re: Review Request for JDK-7107905: ColorModel subclasses are missing hashCode() or equals() or both methods
>
> Please post the updated webrev.
>
> -phil.
>
> On 6/1/16, 12:02 AM, Jayathirth D V wrote:
>> Hi Phil&  Jim,
>>
>> Collating all the changes needed:
>> 1) I have removed both equals()&  hashCode() in CCM but forgot to add the file while creating webrev. I will include changes in CCM in updated webrev.
>> 2) Caching of hashCode value in IndexColorModel&  PackedColorModel :
>> We can cache hashCode value when constructor is called or when hashCode() is called for first time. What approach we have to follow?
>> 3) Comment section of equals() method, I will update it to :
>> 1449      * the target object must be the same class (and not a subclass) as this
>> 4) I will use .equals() to compare colorSpace values in CM.equals() so it will be like we are not intending ColorSpace class to never override equals() method.
>>
>> Please let me know your inputs.
>>
>> Thanks,
>> Jay
>>
>> -----Original Message-----
>> From: Jim Graham
>> Sent: Wednesday, June 01, 2016 4:14 AM
>> To: Phil Race
>> Cc: Jayathirth D V; [hidden email]
>> Subject: Re: Review Request for JDK-7107905: ColorModel subclasses are
>> missing hashCode() or equals() or both methods
>>
>> I think we should use .equals() here, otherwise it looks like a recommendation that the intent is for those classes to never implement it...
>>
>> ...jim
>>
>> On 05/31/2016 03:31 PM, Phil Race wrote:
>>> I don't know of any design intent, so yes, I meant more as a
>>> practical matter.
>>> Unless they subclass then using equals() which I thought unlikely it
>>> makes no difference here.
>>> But it would be proof against that to use equals, unlikely to matter
>>> as it might be ..
>>>
>>> -phil.
>>>
>>> On 05/31/2016 03:19 PM, Jim Graham wrote:
>>>> On 05/31/2016 02:50 PM, Phil Race wrote:
>>>>> On 05/31/2016 12:20 PM, Jim Graham wrote:
>>>>>> Hi Jay,
>>>>>>
>>>>>> You were going to remove hashCode/equals from CCM, but instead you
>>>>>> simply removed it from the patch.  You still need to edit it to
>>>>>> remove the existing methods.
>>>>> Oh yeah ! CCM is gone from the latest webrev. I expect that was not
>>>>> intentional.
>>>>>
>>>>>> Also, I'm not sure == is a good way to compare ColorSpace
>>>>>> instances
>>>>>> - Phil?
>>>>> Neither ColorSpace nor ICC_ColorSpace over-ride equals or hashCode
>>>>> and all the predefined ones are constructed as singletons so it
>>>>> seems unlikely to cause problems
>>>> Should it use .equals() on principle, though?  Otherwise we are
>>>> baking in the assumption that it doesn't implement equals() into our
>>>> implementation of the CM.equals() method.  Might it some day
>>>> implement equals (perhaps it isn't a practical issue today, but it
>>>> might be in the future when we forget that it was omitted in this
>>>> usage we create today).
>>>>
>>>> I guess what I'm asking is if it is a design feature that different
>>>> objects are considered non-equal (such as an object that, for
>>>> instance, has only predetermined instances that are shared by
>>>> reference and reused).  I think color spaces are sort of in-between
>>>> in that we define a few constants that simply get used by reference
>>>> in 99% of cases, but that isn't a design feature, more like a
>>>> practical issue...
>>>>
>>>>               ...jim

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Review Request for JDK-7107905: ColorModel subclasses are missing hashCode() or equals() or both methods

Jim Graham-5
In reply to this post by Jayathirth D v
I just noticed a hashCode/equals violation that we created a few revisions ago.  We compute the hash of the validBits in
ICM, but we only compare validBits up to the number of colors in the colormap.  One could construct two ICMs that have
different validBits that are identical in the first N bits (N = number of colors), but have different bits beyond that,
and those 2 ICMs would evaluate as equals(), but their hashcodes would be different.

Possible solutions:

- Truncate validBits when it is accepted in the constructor (validBits.and(...))
- Manually compute the hash of the first N bits of validBits
- Not use validBits in the hash code since it is allowed to omit significant data from the hash

(Note that everything in hashcode must participate in equals(), but not vice versa)

The same is true of nBits in ColorModel.  (nBits can be copied and truncated with Arrays.copyOf)

The same is *not* true of maskBits in PCM since the constructor creates an array of the precise length it needs...

                        ...jim

On 6/2/16 7:07 AM, Jayathirth D V wrote:

> Hi Phil,
>
> I have updated the code with all the changes I mentioned previously. I am caching hashCode when first time hashCode() is called.
> Please find the updated webrev for review:
> http://cr.openjdk.java.net/~jdv/7107905/webrev.09/
>
> Thanks,
> Jay
>
> -----Original Message-----
> From: Philip Race
> Sent: Wednesday, June 01, 2016 10:06 PM
> To: Jayathirth D V
> Cc: Jim Graham; [hidden email]
> Subject: Re: Review Request for JDK-7107905: ColorModel subclasses are missing hashCode() or equals() or both methods
>
> Please post the updated webrev.
>
> -phil.
>
> On 6/1/16, 12:02 AM, Jayathirth D V wrote:
>> Hi Phil&  Jim,
>>
>> Collating all the changes needed:
>> 1) I have removed both equals()&  hashCode() in CCM but forgot to add the file while creating webrev. I will include changes in CCM in updated webrev.
>> 2) Caching of hashCode value in IndexColorModel&  PackedColorModel :
>> We can cache hashCode value when constructor is called or when hashCode() is called for first time. What approach we have to follow?
>> 3) Comment section of equals() method, I will update it to :
>> 1449      * the target object must be the same class (and not a subclass) as this
>> 4) I will use .equals() to compare colorSpace values in CM.equals() so it will be like we are not intending ColorSpace class to never override equals() method.
>>
>> Please let me know your inputs.
>>
>> Thanks,
>> Jay
>>
>> -----Original Message-----
>> From: Jim Graham
>> Sent: Wednesday, June 01, 2016 4:14 AM
>> To: Phil Race
>> Cc: Jayathirth D V; [hidden email]
>> Subject: Re: Review Request for JDK-7107905: ColorModel subclasses are
>> missing hashCode() or equals() or both methods
>>
>> I think we should use .equals() here, otherwise it looks like a recommendation that the intent is for those classes to never implement it...
>>
>> ...jim
>>
>> On 05/31/2016 03:31 PM, Phil Race wrote:
>>> I don't know of any design intent, so yes, I meant more as a
>>> practical matter.
>>> Unless they subclass then using equals() which I thought unlikely it
>>> makes no difference here.
>>> But it would be proof against that to use equals, unlikely to matter
>>> as it might be ..
>>>
>>> -phil.
>>>
>>> On 05/31/2016 03:19 PM, Jim Graham wrote:
>>>>
>>>> On 05/31/2016 02:50 PM, Phil Race wrote:
>>>>> On 05/31/2016 12:20 PM, Jim Graham wrote:
>>>>>> Hi Jay,
>>>>>>
>>>>>> You were going to remove hashCode/equals from CCM, but instead you
>>>>>> simply removed it from the patch.  You still need to edit it to
>>>>>> remove the existing methods.
>>>>> Oh yeah ! CCM is gone from the latest webrev. I expect that was not
>>>>> intentional.
>>>>>
>>>>>> Also, I'm not sure == is a good way to compare ColorSpace
>>>>>> instances
>>>>>> - Phil?
>>>>> Neither ColorSpace nor ICC_ColorSpace over-ride equals or hashCode
>>>>> and all the predefined ones are constructed as singletons so it
>>>>> seems unlikely to cause problems
>>>> Should it use .equals() on principle, though?  Otherwise we are
>>>> baking in the assumption that it doesn't implement equals() into our
>>>> implementation of the CM.equals() method.  Might it some day
>>>> implement equals (perhaps it isn't a practical issue today, but it
>>>> might be in the future when we forget that it was omitted in this
>>>> usage we create today).
>>>>
>>>> I guess what I'm asking is if it is a design feature that different
>>>> objects are considered non-equal (such as an object that, for
>>>> instance, has only predetermined instances that are shared by
>>>> reference and reused).  I think color spaces are sort of in-between
>>>> in that we define a few constants that simply get used by reference
>>>> in 99% of cases, but that isn't a design feature, more like a
>>>> practical issue...
>>>>
>>>>              ...jim
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Review Request for JDK-7107905: ColorModel subclasses are missing hashCode() or equals() or both methods

Jayathirth D v
Hi Jim,

Oh that's an important change.
Thanks for your review.

I have updated ColorModel constructor to copy nBIts only of numOfComponents length and I have removed validBits check in hashCode() of ICM.
Please find updated webrev for review:
http://cr.openjdk.java.net/~jdv/7107905/webrev.10/ 

Thanks,
Jay

-----Original Message-----
From: Jim Graham
Sent: Friday, June 03, 2016 2:25 AM
To: Jayathirth D V; Philip Race
Cc: [hidden email]
Subject: Re: Review Request for JDK-7107905: ColorModel subclasses are missing hashCode() or equals() or both methods

I just noticed a hashCode/equals violation that we created a few revisions ago.  We compute the hash of the validBits in ICM, but we only compare validBits up to the number of colors in the colormap.  One could construct two ICMs that have different validBits that are identical in the first N bits (N = number of colors), but have different bits beyond that, and those 2 ICMs would evaluate as equals(), but their hashcodes would be different.

Possible solutions:

- Truncate validBits when it is accepted in the constructor (validBits.and(...))
- Manually compute the hash of the first N bits of validBits
- Not use validBits in the hash code since it is allowed to omit significant data from the hash

(Note that everything in hashcode must participate in equals(), but not vice versa)

The same is true of nBits in ColorModel.  (nBits can be copied and truncated with Arrays.copyOf)

The same is *not* true of maskBits in PCM since the constructor creates an array of the precise length it needs...

                        ...jim

On 6/2/16 7:07 AM, Jayathirth D V wrote:

> Hi Phil,
>
> I have updated the code with all the changes I mentioned previously. I am caching hashCode when first time hashCode() is called.
> Please find the updated webrev for review:
> http://cr.openjdk.java.net/~jdv/7107905/webrev.09/
>
> Thanks,
> Jay
>
> -----Original Message-----
> From: Philip Race
> Sent: Wednesday, June 01, 2016 10:06 PM
> To: Jayathirth D V
> Cc: Jim Graham; [hidden email]
> Subject: Re: Review Request for JDK-7107905: ColorModel subclasses are
> missing hashCode() or equals() or both methods
>
> Please post the updated webrev.
>
> -phil.
>
> On 6/1/16, 12:02 AM, Jayathirth D V wrote:
>> Hi Phil&  Jim,
>>
>> Collating all the changes needed:
>> 1) I have removed both equals()&  hashCode() in CCM but forgot to add the file while creating webrev. I will include changes in CCM in updated webrev.
>> 2) Caching of hashCode value in IndexColorModel&  PackedColorModel :
>> We can cache hashCode value when constructor is called or when hashCode() is called for first time. What approach we have to follow?
>> 3) Comment section of equals() method, I will update it to :
>> 1449      * the target object must be the same class (and not a subclass) as this
>> 4) I will use .equals() to compare colorSpace values in CM.equals() so it will be like we are not intending ColorSpace class to never override equals() method.
>>
>> Please let me know your inputs.
>>
>> Thanks,
>> Jay
>>
>> -----Original Message-----
>> From: Jim Graham
>> Sent: Wednesday, June 01, 2016 4:14 AM
>> To: Phil Race
>> Cc: Jayathirth D V; [hidden email]
>> Subject: Re: Review Request for JDK-7107905: ColorModel subclasses
>> are missing hashCode() or equals() or both methods
>>
>> I think we should use .equals() here, otherwise it looks like a recommendation that the intent is for those classes to never implement it...
>>
>> ...jim
>>
>> On 05/31/2016 03:31 PM, Phil Race wrote:
>>> I don't know of any design intent, so yes, I meant more as a
>>> practical matter.
>>> Unless they subclass then using equals() which I thought unlikely it
>>> makes no difference here.
>>> But it would be proof against that to use equals, unlikely to matter
>>> as it might be ..
>>>
>>> -phil.
>>>
>>> On 05/31/2016 03:19 PM, Jim Graham wrote:
>>>>
>>>> On 05/31/2016 02:50 PM, Phil Race wrote:
>>>>> On 05/31/2016 12:20 PM, Jim Graham wrote:
>>>>>> Hi Jay,
>>>>>>
>>>>>> You were going to remove hashCode/equals from CCM, but instead
>>>>>> you simply removed it from the patch.  You still need to edit it
>>>>>> to remove the existing methods.
>>>>> Oh yeah ! CCM is gone from the latest webrev. I expect that was
>>>>> not intentional.
>>>>>
>>>>>> Also, I'm not sure == is a good way to compare ColorSpace
>>>>>> instances
>>>>>> - Phil?
>>>>> Neither ColorSpace nor ICC_ColorSpace over-ride equals or hashCode
>>>>> and all the predefined ones are constructed as singletons so it
>>>>> seems unlikely to cause problems
>>>> Should it use .equals() on principle, though?  Otherwise we are
>>>> baking in the assumption that it doesn't implement equals() into
>>>> our implementation of the CM.equals() method.  Might it some day
>>>> implement equals (perhaps it isn't a practical issue today, but it
>>>> might be in the future when we forget that it was omitted in this
>>>> usage we create today).
>>>>
>>>> I guess what I'm asking is if it is a design feature that different
>>>> objects are considered non-equal (such as an object that, for
>>>> instance, has only predetermined instances that are shared by
>>>> reference and reused).  I think color spaces are sort of in-between
>>>> in that we define a few constants that simply get used by reference
>>>> in 99% of cases, but that isn't a design feature, more like a
>>>> practical issue...
>>>>
>>>>              ...jim
123
Loading...