RFR: 8152844: JVM InstanceKlass Methods For Obtaining Package/Module Should Be Moved to Klass
Rachel Protacio
rachel.protacio at oracle.com
Mon Apr 25 13:18:33 UTC 2016
Hi,
No, that's fine. Here's the updated webrev:
http://cr.openjdk.java.net/~rprotacio/8152844.01/
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
>>>
>>
More information about the hotspot-runtime-dev
mailing list