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

[David Holmes]

On 7/06/2016 12:32 PM, David Holmes wrote:
> On 7/06/2016 4:52 AM, 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);
>
> Which is why I said you could only add tests for return paths prior to
> that point.

Sorry I misunderstood what you were referring to. You can't create the 
Symbol to pass in without also having a THREAD reference. Okay that is a 
pity.

Thanks,
David

> "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..."
>
> David
> -----
>
>> 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
>>>>
>>