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

[Rachel Protacio]

Hi,

Thanks for the comments. New webrev: 
http://cr.openjdk.java.net/~rprotacio/8153858.01/

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