Request Review: JDK-6479237 (cl) Add support for classloader names
mandy.chung at oracle.com
Tue Nov 1 23:42:18 UTC 2016
Here is the updated webrev incorporating your feedback:
> On Nov 1, 2016, at 7:04 AM, Daniel Fuchs <daniel.fuchs at oracle.com> wrote:
> 334 s += declaringClass;
> but should line 334 instead be
> s = (s.isEmpty() ? declaringClass : s + "/" + declaringClass;
> Otherwise "/" will be missing after module at version..
Good catch. I added a test for this.
> Also should there be some asserts somewhere verifying that moduleVersion
> is null or empty when moduleName is null or empty? At the moment the
> constructor will blindly accept a version for an unnamed module,
> and I am assuming this is wrong - am I right?
Good point. The constructor should throw IAE if module name/version and class loader name is an empty string.
> toLoaderModuleClassName will call ClassLoader.getName().
> If a class loader overrides getName() then that might be a different
> name than what StackTraceElement.getClassLoaderName() returns.
> Is that an issue?
I added a test to verify StackTraceElement that uses the ClassLoader's name field. I also added @apiNote in ClassLoader::getName.
> Also I wonder whether you should be considering including a fix
> for https://bugs.openjdk.java.net/browse/JDK-8167099 as part
> of this change (though arguably the bug has been there since
> new fields were added to StackTraceElement in 9).
It is related but it’s better to separate it from this issue. Do you have cycle to help write a test case? I can help fix JDK-8167099 later.
More information about the hotspot-runtime-dev