RFR(XS): 8165613: Convert TestKlass_test to Gtest
Coleen Phillimore
coleen.phillimore at oracle.com
Thu Sep 15 16:06:56 UTC 2016
On 9/15/16 10:53 AM, Kirill Zhaldybin wrote:
> Coleen,
>
> Thank you for reviewing the fix!
>
> Here are a new WebRev:
> http://cr.openjdk.java.net/~kzhaldyb/webrevs/JDK-8165613/webrev.01/
>
> I changed the test name and moved it to the appropriate location.
>
> Could you please let me know your opinion?
I think I figured out the point of this test. It's apparently trying to
add a test for the InstanceKlass::is_class_loader_instance_klass() function.
So I have to admit that I was wrong to have you move it to
systemDictionary, since the function that it's trying to test is in
InstanceKlass, so it should belong to InstanceKlass.
28 TEST_VM(SystemDictionary, class_loader_class) {
29 Klass* klass = SystemDictionary::ClassLoader_klass();
30 ASSERT_TRUE(klass->is_instance_klass());
31 ASSERT_TRUE(InstanceKlass::cast(klass)->is_class_loader_instance_klass());
32 }
33
34 TEST_VM(SystemDictionary, string_klass) {
35 Klass* klass = SystemDictionary::String_klass();
36 ASSERT_TRUE(!klass->is_instance_klass() ||
37 !InstanceKlass::cast(klass)->is_class_loader_instance_klass());
Since SystemDictionary::x_klass() now returns InstanceKlass, this can be written as:
28 TEST_VM(InstanceKlass, class_loader_class) {
29 InstanceKlass* klass = SystemDictionary::ClassLoader_klass();
31 ASSERT_TRUE(klass->is_class_loader_instance_klass());
32 }
33
34 TEST_VM(InstanceKlass, string_klass) {
35 InstanceKlass* klass = SystemDictionary::String_klass();
36 ASSERT_TRUE(!klass->is_class_loader_instance_klass());
And then add a comment that this is the point of the test since I had to
go back to history to figure this out. I'm sorry that I didn't research
this when I made my original comments. Thanks, Coleen
> Thank you. Regards, Kirill On 12.09.2016 15:28, Coleen Phillimore wrote:
>> This looks good but I'm confused about something:
>> http://cr.openjdk.java.net/~kzhaldyb/webrevs/JDK-8165613/webrev.00/test/native/oops/test_klass.cpp.html
>> 28 TEST_VM(SystemDictionary, class_loader_class) { It seems like
>> making the tests test the SystemDictionary is more appropriate than
>> InstanceKlass, which I think this does. If so, can you move the test
>> file to the directory test/native/classfile/test_systemDictionary.cpp
>> (or whatever the convention is), if it exists? Thanks, Coleen On
>> 9/12/16 8:15 AM, Kirill Zhaldbybin wrote:
>>> Robert, Thank you for review! Regards, Kirill On 09/12/2016 11:35
>>> AM, Robbin Ehn wrote:
>>>> Looks good. Thanks! /Robbin (not a 'R'eviewer) On 09/08/2016 04:48
>>>> PM, Kirill Zhaldybin wrote:
>>>>> Dear all, Could you please review this fix for 8165613? WebRev:
>>>>> http://cr.openjdk.java.net/~kzhaldyb/webrevs/JDK-8165613/webrev.00/
>>>>> CR: https://bugs.openjdk.java.net/browse/JDK-8165613 Thank you.
>>>>> Regards, Kirill
More information about the hotspot-runtime-dev
mailing list