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

[Coleen Phillimore]

Hi,

I missed some of the iterations of this. To avoid making a large change 
larger, I think you should keep the original names of 
ToStackTraceElement and GetStackTraceElements.  I don't think the new 
names are better and it leads to confusion of where the names are.  It 
breaks the (hopefully ubiquitous) convention of naming Java functions 
similarly to the JVM functions that implement them.

+JNIEXPORT void JNICALL Java_java_lang_StackTraceElement_fillFromStackFrameInfo
+  (JNIEnv *env, jobject element, jobject stackframeinfo) {
+     JVM_ToStackTraceElement(env, stackframeinfo, element);
+}
+
+JNIEXPORT void JNICALL Java_java_lang_StackTraceElement_fillStackTraceFromThrowable
+  (JNIEnv *env, jobject dummy, jobjectArray elements, jobject throwable)
+{
+    JVM_GetStackTraceElements(env, throwable, elements);
+}

I had already filed a compatibility request using these names to replace 
the old ones.   If you want to change them again, you should file a new 
compatibility request to change the old jdk8 name (which I don't 
remember right now).

http://cr.openjdk.java.net/~mchung/jdk9/webrevs/6479237/webrev.03/jdk/src/java.base/share/classes/java/lang/StackFrameInfo.java.udiff.html

Maybe you can use the name "fill" for this rather than "of".   If you 
don't use an IDE, finding the function name "of" is really challenging.

Also, I think you should check this into the hs repository when done so 
that this bug can be unblocked sooner: 
https://bugs.openjdk.java.net/browse/JDK-8165550.

Thanks,
Coleen


On 11/1/16 7:42 PM, Mandy Chung wrote:
> 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 <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.
>
> Mandy
>