RFR 8029567: Clean up linkResolver code

[Lois Foltan]

On 5/14/2015 7:16 PM, Coleen Phillimore wrote:
> Summary: Moved non-const reference return values to actual return 
> values, refactored error handling code, pass CLD rather than 
> class_loader oop, remove oop from Method* variable names.
>
> Also modified the long parameter lists into a class LinkInfo to hold 
> information from the constant pool that is always passed through 
> several layers of functions together.  Also reformatted to split 
> parameter list lines to some reasonable width.
>
> The type methodHandle should be passed as const reference types to 
> avoid copy construction, because it has a non-trivial destructor. This 
> sort of change could be made in more places in the JVM, but I stopped 
> with linkResolver.
>
> Ran all hotspot jtreg tests, jck lang/vm/api/java_lang tests, internal 
> testbase tests: vm.quick.testlist, vm.defmeth.testlist, proposed new 
> selection-resolution tests, jdk/java/lang/invoke tests (see RFR).
>
> open webrev at http://cr.openjdk.java.net/~coleenp/8029567/
> bug link https://bugs.openjdk.java.net/browse/JDK-8029567

Hi Coleen,

Looks like a great clean up, few comments, nothing major though:

src/share/vm/classfile/loaderConstraints.[c/h]pp, systemDictionary.[c/h]pp
- I would prefer consistently changing the parameter name as well to 
"loader_data".  I associate "loader" with a ClassLoader.

src/share/vm/interpreter/linkResolver.hpp
- line #153 - would you consider moving the body of method_string into 
the hpp file since it is straight forward.
- Can you add a LinkInfo::print() method?

src/share/vm/interpreter/linkResolver.cpp
- line #264 - I like the readability of returning a methodHandle result 
instead of void in LinkResolver::lookup_method_in_klasses, but just for 
my clarifications the result was being passed in by reference to avoid a 
copy ctor.  Now, there is a copy ctor occurring on the return value, 
correct?

- line #496, LinkResolver::resolve_method_statically formerly passed 
both resolved_method and resolved_klass by reference.  So changing your 
change implies that resolved_klass no longer needs the changes to be 
reflected in it post call?  For example, the Bytecodes::_invokedynamic 
if statement does reassign resolved_klass and returns with 
resolved_klass having that new value?

- line #726 why set current_klass if link_info.check_access() is false, 
could move that assignment into the if statement.  Or in line #753, 
instead of using link_info().current_klass() use "current_klass".

- line #876 - same sort of comment as line #496 - resolved_klass could 
have been altered and the value changed post call.  So I  am assuming 
that is why the assert at line #889

src/share/vm/oops/klassVtable.cpp
- line 447 & 449, 1168 & 1170 -  can you rename loader1 and loader2 to 
loader1_name and loader2_name, this is a nit comment but I find the use 
of a variable named "loader" to be very overloaded.

Thanks,
Lois

>
> Thanks,
> Coleen