Hi Daniel, Here is the updated webrev incorporating your feedback: http://cr.openjdk.java.net/~mchung/jdk9/webrevs/6479237/webrev.03
> On Nov 1, 2016, at 7:04 AM, Daniel Fuchs <[email protected]> wrote: > > : > 334 s += declaringClass; > > > but should line 334 instead be > > s = (s.isEmpty() ? declaringClass : s + "/" + declaringClass; > > Otherwise "/" will be missing after module@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. Mandy
