Code Review Request: 8016698: Cleanup overrides warning in sun/tools/ClassDeclaration.java (original) (raw)

Kurchi Hazra kurchi.subhra.hazra at oracle.com
Tue Jun 18 18:46:25 UTC 2013

Yes, I have that removed. Thanks for the review!

On 6/18/2013 1:57 AM, Chris Hegarty wrote: > For the purposes of this issue, I think your changes to hashCode are > in line with the equals implementation. Trivially, you could also > remove the '(obj != null) && ' from equals ;-) Otherwise, looks fine > to me. >> -Chris. >> On 06/18/2013 12:01 AM, Kurchi Hazra wrote: >> 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/viewbug.do?bugid=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 >>

-Kurchi

More information about the core-libs-dev mailing list