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

Calvin Cheung calvin.cheung at oracle.com
Wed Apr 15 23:25:56 UTC 2015

Hi Stefan,

Thanks for your review.

On 4/15/2015 2:50 PM, Stefan Karlsson wrote:

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 notequals: - if (name != vmSymbols::objectinitializername()) { + if (name->notequals(vmSymbols::objectinitializername())) { instead of just: + if (!name->equals(vmSymbols::objectinitializername())) { We think that it's clearer to have not_equals() than using the ! as in the above. http://cr.openjdk.java.net/~ccheung/8071627/webrev.01/src/share/vm/classfile/systemDictionary.cpp.udiff.html ! assert(parsedname->notequals(NULL), "Sanity"); You use symbol == NULL but not symbol != NULL, which seems inconsistent to me. I'm not sure I understand this comment. Are you referring to the following section of the udiff? *** 1104,1115 **** --- 1104,1115 ---- Exceptions::_throw_msg(THREAD_AND_LOCATION, vmSymbols::java_lang_SecurityException(), message); }

 if (!HAS_PENDING_EXCEPTION) {

! assert(parsed_name != NULL, "Sanity"); ! assert(class_name == NULL || class_name == parsed_name, "name mismatch"); ! assert(parsed_name->not_equals(NULL), "Sanity"); ! assert(class_name == NULL || class_name->equals(parsed_name), "name mismatch"); // Verification prevents us from creating names with dots in them, this // asserts that that's the case. assert(is_internal_format(parsed_name), "external class name format used internally");

I don't see anything incorrect there.

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

I will fix it.

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?

Yes. Essentially comment out the bodies of the == and != operators in SymbolRef. Rebuilding hotspot resulting a lot of "undefined reference to SymbolRef::operator==" link errors. Then go through those error and change a == b to a->equals(b) and a != b to a->not_equals(b). Obviously change SymbolRef back to Symbol*.

thanks, Calvin

Thanks, StefanK

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

More information about the hotspot-dev mailing list