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

[David Holmes]

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"

---

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?

---

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;

---

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?

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.


> 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!


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