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

[Mandy Chung]

> On Nov 2, 2016, at 5:40 PM, Coleen Phillimore <coleen.phillimore at oracle.com> wrote:
> 
> 
> 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.


There is no change to the name of these JVM entry points.

This patch changes the private native methods in the library.  Is that what concerns you?

Mandy

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