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

[Phil Race]

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.

-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
>