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@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.christ...@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
