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

[Stefan Karlsson]

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 not_equals()) 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:

-              if (name != vmSymbols::object_initializer_name()) {
+              if (name->not_equals(vmSymbols::object_initializer_name())) {


instead of just:

+              if (!name->equals(vmSymbols::object_initializer_name())) {


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