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

[Rachel Protacio]

Also, I updated systemDictionary.cpp at line 1161 to use 
StringUtils::replace_no_expand() instead of stepping through the string. 
http://cr.openjdk.java.net/~rprotacio/8153858.02/

Rachel


On 6/6/2016 2:52 PM, Rachel Protacio wrote:
> 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
>>>
>