RFR: 8152844: JVM InstanceKlass Methods For Obtaining Package/Module Should Be Moved to Klass

[Lois Foltan]

On 4/21/2016 4:11 PM, Rachel Protacio wrote:
> Hello,
>
> Please review this fix for the package() and module() functions, 
> making them pure virtual functions of Klass and implementing them in 
> the relevant child classes. This cleans up some verbose and repetitive 
> code that was casting and checking klass types. Also added test cases 
> to runtime/modules/getModuleJNI/GetModule.java, ensuring that all the 
> module() functions work as expected.
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8152844
> Open webrev: http://cr.openjdk.java.net/~rprotacio/8152844.00/

Hi Rachel,

This looks really good!  A couple of comments:

- src/share/vm/oops/arrayKlass.hpp
line #140 & 141 - instead of inserting the declarations of the module 
and package methods at the end of the ArrayKlass data structure can you 
move them up to line #71 or right after the declaration of java_super() 
at line #73.  This is picky, but for readability seems to fit more with 
other like methods.

- src/share/vm/oops/klass.hpp
It seems like the precedence is to have forward declarations for classes 
that the Klass data structures uses.  Can you change that for 
PackageEntry and ModuleEntry instead of including the headers?

- src/share/vm/oops/objArrayKlass.cpp
line #420 - Remove //Packages comment - really adds no value
line #421 - Remove blank line
#line #423 - A comment that would be helpful and please include, "The 
array is defined in the module of its bottom class"
// The array is defined in the module of its bottom class
- src/share/vm/oops/typeArrayKlass.cpp
line #348 - A comment is needed, something like, "A TypeArrayKlass is an 
array of a primitive type, its defining module is java.base"

- src/share/vm/runtime/reflection.cpp
line #511 & #512 - I think we need to change that comment because I am 
not sure if it is accurate to state that java.base exports its packages 
to all modules.  Please change to something like, "A TypeArray's 
defining module is java.base, access to the TypeArray is allowed"

- test/runtime/modules/getModuleJNI/GetModule.java
Thank you for improving this test!

I don't need to see another webrev for this,
Thanks,
Lois

>
> Tested with jtreg tests in jdk/test (java/lang, java/io, 
> java/security, java/util, tools/jimage, tools/jlink, and tools/jmod), 
> JPRT, and RBT hotspot and non-colo tests.
>
> Thanks!
> Rachel