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

[David Holmes]

On 25/04/2016 11:18 PM, Rachel Protacio wrote:
> Hi,
>
> No, that's fine. Here's the updated webrev:
> http://cr.openjdk.java.net/~rprotacio/8152844.01/

Thanks - looks good.

David

> Rachel
>
> On 4/25/2016 1:37 AM, David Holmes wrote:
>> Hi Rachel,
>>
>> Maybe too late, but despite comments to the contrary from the other
>> reviewers I think there are enough suggested changes being suggested
>> that I would want to see another webrev.
>>
>> Thanks,
>> David
>>
>> On 23/04/2016 1:22 AM, Rachel Protacio wrote:
>>> Hi, Coleen,
>>>
>>> Thanks for the review! I'll remove the arrayKlass entries and, seeing no
>>> other objections, will commit.
>>>
>>> Rachel
>>>
>>> On 4/22/2016 9:24 AM, Coleen Phillimore wrote:
>>>>
>>>> Hi,
>>>>
>>>> http://cr.openjdk.java.net/~rprotacio/8152844.00/src/share/vm/oops/arrayKlass.cpp.udiff.html
>>>>
>>>>
>>>>
>>>> I don't think you need module or package declared in arrayKlass.hpp
>>>> since it should be abstract also, ie. there should be no instances of
>>>> ArrayKlass.
>>>>
>>>> Otherwise, looks great.  I love seeing those InstanceKlass::casts go
>>>> away.
>>>>
>>>> Coleen
>>>>
>>>>
>>>> On 4/21/16 10:26 PM, David Holmes wrote:
>>>>> Hi Rachel,
>>>>>
>>>>> On 22/04/2016 6:11 AM, 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/
>>>>>
>>>>> The refactoring all looks good to me (though C++ annoys me in needing
>>>>> to re-declare the functions in each subclass :) ).
>>>>>
>>>>> Cheers,
>>>>> David
>>>>>
>>>>>> 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
>>>>
>>>
>