Code Review Request: 8016698: Cleanup overrides warning in sun/tools/ClassDeclaration.java

[Kurchi Hazra]

I spent some more time reviewing this, I believe the equals() 
implementation is correct. Given one string, we get a unique
Identifier. The Identifier has the associated Type cached - so given an 
Identifier, we get a unique Type too. One usage of the
ClassDeclaration#equals() method is in ClassDefinition#subClassOf(), 
which walks up the inheritance path of the concerned
class to check if any of those are equal to the argument 
ClassDeclaration (and hence have equal Types)..

- Kurchi

On 6/17/2013 10:45 AM, Kurchi Hazra wrote:
>
> On 6/14/2013 11:56 PM, Alan Bateman wrote:
>> On 14/06/2013 23:59, Kurchi Hazra wrote:
>>>
>>> Hi,
>>> This is to elimnate the overrides warning from 
>>> ClassDeclaration.java. This class is used by rmi, but sun/tools/java 
>>> and
>>> sun/tools/javac also heavily uses it, let me know if anyone else 
>>> should also be reviewing it.
>>>
>>> Bug: http://bugs.sun.com/view_bug.do?bug_id=8016698 [To appear]
>>> Webrev: http://cr.openjdk.java.net/~khazra/8016698/webrev.00/
>>>
>>> Thanks,
>>> Kurchi
>> This looks okay for the purposes of squashing the warning but the 
>> existing equals method does look suspect. It might not matter for the 
>> rmic usage but as the equals doesn't take account of the the 
>> "definition" then it looks to me that it consider very different 
>> ClassDeclarations as equals. It would be good to understand how it is 
>> used to see whether we have an issue here or not.
> Thanks for the review.
> If you analyse "definition", it is not constant and changes as the 
> state of the class changes (whether binary class is loaded, whether it
> has been compiled and so on) - and so does the status field. The type 
> field looks like the only good candidate to use in the equals method -
> so the equals() looks correct to me.
> Now the question is whether there is any need of overriding 
> Object.equals() at all, that is, whether each ClassDeclaration object 
> should
> be treated as a unique one - I am looking around in the source code to 
> check that.
>
> Thanks,
> - Kurchi