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

[Rachel Protacio]

Hi David,

I see what you're suggesting with the test, but since there is no THREAD 
to use in the gtest, I can't for example write
     TempNewSymbol sym = SymbolTable::new_symbol("", THREAD);
to then pass into InstanceKlass::package_from_name. On the other hand, 
creating a new_symbol("", NULL) succeeds I think because it looks up the 
symbol with the empty string name which happens to exist in the table, 
but in reality it cannot create a new symbol on a non-existent thread. 
So even though it would be good to test, I don't think there's a way to 
do it. What do you think?

Thanks,
Rachel

On 6/5/2016 9:15 PM, David Holmes wrote:
> 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
>>