[OpenJDK 2D-Dev] 8017109: Cleanup overrides warning in src/solaris/classes/sun/print/AttributeClass.java

[Chris Hegarty]

On 20/06/2013 22:59, Phil Race wrote:
> On 6/20/2013 11:54 AM, Kurchi Hazra wrote:
>> Thanks Chris, webrev with the equals() changes then :
>> http://cr.openjdk.java.net/~khazra/8017109/webrev.01/
>> I am running this through jdk_awt and jdk_swing tests. If all is good,
>> I'll push through tl then.
>
> Not sure which tests those are.
> I think the only way this will get exercised is if you run it through
> printing tests on Linux or Solaris 11 as this code is used only
> by CUPS printing. Anything else you do is probably a waste of time.
> I think automated tests will tell you very little as they are often
> run on systems without printers and may not even hit this code,
> unless they actually print.
>
> I am not sure why equals is only testing the name, but it looks
> like the name is used as a hashmap key with instances as values
> so if there's more than one with differences in the other fields,
> there are bigger problems.
>
> Really I'd prefer you push this via 2d (not awt as Chris suggested)
> but I won't insist if it will cause pain for you.

Sorry, my confusion, 2d would be better.

I don't think it really matters to Kurchi where this gets pushed, so 
long as there is some advantage of getting it into 2d, rather than tl ( 
more testing before integration, PIT, etc ).

Alternatively, someone from the "2d" area could sponsor Kurchi's 
changeset into 2d.

-Chris.

>
> -phil.
>
>
>>
>> Thanks,
>> - Kurchi
>>
>> On 6/20/2013 1:37 AM, Chris Hegarty wrote:
>>> On 06/19/2013 10:38 PM, Kurchi Hazra wrote:
>>>> Hi,
>>>>
>>>> The class src/solaris/classes/sun/print/AttributeClass.java overrides
>>>> Object.equals() but not Object.hashCode() because of which
>>>> we get a warning everytime we build jdk8/tl. Here is a suggestion to
>>>> remove the warning:
>>>>
>>>> http://cr.openjdk.java.net/~khazra/8017109/webrev.00/
>>>> <http://cr.openjdk.java.net/%7Ekhazra/8017109/webrev.00/>
>>>
>>> Your changes to hashCode are in line with what the equals method is
>>> doing. So, they look fine to me.
>>>
>>>> I was also wondering whether the implementation of the equals() method
>>>> is correct, and whether it is instead meant to be:
>>>>
>>>> diff -r eb1a3c50a2a9 src/solaris/classes/sun/print/AttributeClass.java
>>>> --- a/src/solaris/classes/sun/print/AttributeClass.java Tue Jun 18
>>>> 14:11:45 2013 -0700
>>>> +++ b/src/solaris/classes/sun/print/AttributeClass.java Wed Jun 19
>>>> 14:20:27 2013 -0700
>>>> @@ -250,9 +250,8 @@
>>>>
>>>> public boolean equals(Object obj) {
>>>> return
>>>> - obj != null &&
>>>> obj instanceof AttributeClass &&
>>>> - obj.toString().equals (((AttributeClass) obj).toString());
>>>> + toString().equals (((AttributeClass) obj).toString());
>>>> }
>>>
>>> D'oh!
>>>
>>>>
>>>> public String toString() {
>>>>
>>>> What is the preferable way of getting this change into the code - I can
>>>> push via tl. What are the tests I would need to run?
>>>
>>> It probably should go through the awt forest, but since the scope is
>>> limited I see no problem running it through TL.
>>>
>>> -Chris.
>>>
>>>
>>>> If this discussion belongs elsewhere, let me know.
>>>>
>>>> Thanks for your help,
>>>> - Kurchi
>>
>