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

[Rachel Protacio]

Thanks, Harold!
Rachel

On 4/25/2016 9:37 AM, harold seigel wrote:
> Hi Rachel,
>
> These changes look good!
>
> Thanks, Harold
>
> On 4/25/2016 9:18 AM, Rachel Protacio wrote:
>> 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
>>>>>
>>>>
>>
>