RFR (2nd round) 8071627: Code refactoring to override == operator of Symbol* (original) (raw)

Stefan Karlsson stefan.karlsson at oracle.com
Wed Apr 15 21:50:23 UTC 2015

Hi Calvin,

On 2015-04-15 21:56, Calvin Cheung wrote:

Please review this second version of the fix.

This version has 2 new functions (equals() and notequals()) in the Symbol class. It replaces the Symbol* == and != comparisons with those 2 function calls. Pro: It has a much smaller changeset than the first version. Con: Someone may by mistake introduce a new line of (Symbol* == Symbol*). We will mitigate this by enhancing our internal static analysis tool to flag the above code in the future. JBS: https://bugs.openjdk.java.net/browse/JDK-8071627 webrev: http://cr.openjdk.java.net/~ccheung/8071627/webrev.01/

This is a much less intrusive change than the previous patch. Thanks.

http://cr.openjdk.java.net/~ccheung/8071627/webrev.01/src/share/vm/classfile/classFileParser.cpp.patch

Is there a reason why you added not_equals:

instead of just:

http://cr.openjdk.java.net/~ccheung/8071627/webrev.01/src/share/vm/classfile/systemDictionary.cpp.udiff.html ! assert(parsed_name->not_equals(NULL), "Sanity");

You use symbol == NULL but not symbol != NULL, which seems inconsistent to me.

http://cr.openjdk.java.net/~ccheung/8071627/webrev.01/src/share/vm/oops/symbol.hpp.udiff.html

+ inline bool equals(const Symbol other) const {* *+ if (this && other) { *

First, pointers should be checked null checked with == or !=. See: https://wiki.openjdk.java.net/display/HotSpot/StyleGuide#StyleGuide-Miscellaneous

Second, I recall some discussion that null checking the 'this' pointer is undefined behavior. Though, we do it in other places so this isn't worse, I think.

Did you use your previous patch to find all Symbol* compares?

Thanks, StefanK

Tests: JPRT (almost done) Will do more perf testing after JPRT thanks, Calvin

More information about the hotspot-dev mailing list