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

[Rachel Protacio]

Thanks for the review, Lois. Will make those changes.

Rachel

On 4/22/2016 8:45 AM, Lois Foltan wrote:
>
> 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
>