Request Review: JDK-6479237 (cl) Add support for classloader names

[Daniel Fuchs]

Hi Mandy,

I am sure I must be missing something:

  322         if (s == null) {
  323             // all elements will be included
  324             s = "";
  325             if (classLoaderName != null && 
!classLoaderName.isEmpty()) {
  326                 s += classLoaderName + "/";
  327             }
  328             if (moduleName != null && !moduleName.isEmpty()) {
  329                 s += moduleName;
  330             }
  331             if (moduleVersion != null && !moduleVersion.isEmpty()) {
  332                 s += "@" + moduleVersion;
  333             }
  334             s += declaringClass;
  335         }

but should line 334 instead be

    s = (s.isEmpty() ? declaringClass : s + "/" + declaringClass;

Otherwise "/" will be missing after module at version..

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?

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?

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).


best regards,

-- daniel

On 28/10/16 21:44, Mandy Chung wrote:
>
>> On Oct 28, 2016, at 11:11 AM, Brent Christian <brent.christian at oracle.com> wrote:
>>
>> Should something be done for STEs returned from StackFrameInfo.toStackTraceElement() ?
>
> Good catch - I missed it.  I added package-private static methods in StackTraceElement class for both Throwable and StackFrameInfo to get StackTraceElement(s).
>
> http://cr.openjdk.java.net/~mchung/jdk9/webrevs/6479237/webrev.02/
>
> Mandy
>