RFR: 8153858: Clean up needed when obtaining the package name from a fully qualified class name

[David Holmes]

Hi Rachel,

On 4/06/2016 6:20 AM, Rachel Protacio wrote:
> Hi,
>
> Thanks for the comments. New webrev:
> http://cr.openjdk.java.net/~rprotacio/8153858.01/

All changes look okay to me.

Regarding the test ... you stated that because gtest can't pass THREAD 
that the InstanceKlass functionality is just tested via JCK tests. But 
in fact you do have one test for InstanceKlass::package_from_name where 
you pass in NULL as THREAD. Given the code in package_from_name:

2187 Symbol* InstanceKlass::package_from_name(const Symbol* name, TRAPS) {
2188   if (name == NULL) {
2189     return NULL;
2190   } else {
2191     if (name->utf8_length() <= 0) {
2192       return NULL;
2193     }
2194     ResourceMark rm;
2195     const char* package_name = 
ClassLoader::package_from_name((const char*) name->as_C_string());
2196     if (package_name == NULL) {
2197       return NULL;
2198     }
2199     Symbol* pkg_name = SymbolTable::new_symbol(package_name, THREAD);
2200     return pkg_name;
2201   }
2202 }

You are currently testing the return path at L#2189. In theory you can 
test all return paths until the NULL THREAD would cause a problem. So 
you could have a test to return at L#2192 and L#2197. But given the NULL 
returning calls to ClassLoader::package_from_name are already being 
independently tested, there is no need to duplicate them. So that just 
leaves adding a test for return at L#2192 - which I'm guessing using "" 
as the name would test.

Thanks,
David
-----

> Replies inline.
>
> On 6/3/2016 1:48 AM, David Holmes wrote:
>> Hi Rachel,
>>
>> Overall looks good, but a few comments below.
>>
>> On 3/06/2016 3:12 AM, Rachel Protacio wrote:
>>> Hello,
>>>
>>> Please review this fix, which consolidates/refactors all the instances
>>> of code parsing fully qualified names and returning the package name.
>>> Includes gtests for the functions. The gtest framework does not allow
>>> for passing in a THREAD, so the InstranceKlass functionality is just
>>> tested by jck tests.
>>
>> src/share/vm/classfile/classLoader.hpp
>>
>> Can you document what the bad_class_name boolean indicates please. It
>> took me a while to figure out that
>> InstanceKlass::is_same_class_package (but seemingly no other caller)
>> needs to distinguish between "has no package" and "an error occurred
>> trying to determine what the package is"
>>
> Added.
>> ---
>>
>> src/share/vm/classfile/systemDictionary.cpp
>>
>> 1157     TempNewSymbol pkg_name =
>> InstanceKlass::package_from_name(parsed_name, THREAD);
>>
>> Why are you not using a CHECK macro here?
> My mistake. I've put in a CHECK_NULL.
>>
>> ---
>>
>> src/share/vm/oops/instanceKlass.cpp
>>
>> 2318     bool bad_class_name1 = false;
>> 2324     bool bad_class_name2 = false;
>>
>> You only need a single local variable.
>>
>> 2336     // Check that package is identical
>> 2337     return !strcmp(name1, name2);
>>
>> Please avoid implicit booleans: return strcmp(name1, name2) == 0;
> Ok.
>>
>> ---
>>
>> test/native/runtime/test_instance_klass.cpp
>>
>> Should the naming convention here match the source file that holds the
>> code being tested ie test_instanceKlass and test_classLoader?
> Done.
>>
>> It seems that without being able to pass a Thread you can test a
>> number of inputs, as long as you won't reach the
>> SymbolTable::new_symbol call. That said the call to
>> ClassLoader::package_from_name is already being tested separately, so
>> it probably suffices to just add a test for the utf8_length() check.
> Sorry, I'm not sure what you mean here. Can you elaborate?
>>
>>> Note that the change in method.hpp was a
>>> compilation error uncovered by my gtest.
>>
>> ??? Your change would seem to me to be introducing a potential
>> compilation error:
>>
>> return (sizeof(Method)/wordSize + is_native()) ? 2 : 0;
>>
>> this adds a boolean to an integer. And the result will always be 2!
> Oops, you're right! I've moved the parentheses around the ternary
> statement.
>
> Rachel
>>
>>
>> Thanks,
>> David
>>
>>>
>>> Passes hotspot gtests, jck lang and api/java_lang tests, JPRT, and RBT
>>> hotspot and non-colo tests.
>>>
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8153858
>>> Open webrev: http://cr.openjdk.java.net/~rprotacio/8153858.00/
>>>
>>> Thank you,
>>> Rachel
>