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

[Coleen Phillimore]

On 11/2/16 10:00 PM, Mandy Chung wrote:
>> On Nov 2, 2016, at 6:10 PM, Coleen Phillimore <coleen.phillimore at oracle.com> wrote:
>>
>>
>>
>> On 11/2/16 8:53 PM, Mandy Chung wrote:
>>>> On Nov 2, 2016, at 5:48 PM, Coleen Phillimore <coleen.phillimore at oracle.com> wrote:
>>>>
>>>> Yes, the names of the library methods don't match the JVM names anymore.  I think changing the names are unnecessary and makes it not obvious the mapping.
>>> Is there such convention?  I wasn’t aware of that.  In addition, it would be impractical to synchronize the native methods to match JVM function names.
>> Yes, there's a convention of having very similar names.  It's not 100% but having completely different names here is really not helpful for maintaining this code.   This is from looking at the names in src/java.base/share/native/libjava.  Some of the names have additional namespace-like strings like "Class" but basically they're close to the same.  I think it's a good convention to have.  In this case, I think the names toStackTraceElement and getStackTraceElements are pretty well descriptive and don't see the reason to change them.
>>
>
> This change was motivated due to two different places filling StackTraceElement (Throwable and StackFrameInfo).  So I added the factory methods to allocate and fill in StackTraceElement object(s) in StackTraceElement and removed the package-private no-arg constructor added by JDK-8150778 [1]. Any change to create such factory methods will be in one single place.

This is cool, and nicer encapsulation.
>
> I like toStackTraceElement while getStackTraceElements does not quite represent what it does.  “fillStackTraceFromThrowable” is a more appropriate name.

toStackTraceElement seems good in the context where this is called now.  
I agree that getStackTraceElements is less so.
>
> The original method Throwable::getStackTraceElements fills in the StackTraceElement array with this Throwable.  I think that method can be renamed to “fillStackTraceFromThrowable” which is clearer.
>
> I suggest to rename the JVM function:
>     JVM_GetStackTraceElements => JVM_FillStackTraceElementsFromThrowable
>     JVM_ToStackTraceElement => JVM_FillStackTraceElement
>
> Are you okay with this?

My problem with fillStackTraceFromThrowable, besides it being a really 
long name, is that it's too close to fillInStackTrace which is a known 
API and should not be changed.

Why not keep JVM_ToStackTraceElement, and change 
JVM_GetStackTraceElements if you really need to to 
JVM_ToStackTraceElementArray().

Or even JVM_ToStackTraceElements() which is very close to the function 
that it should conceptually be an overloaded instance.

Throwable is part of the API, so putting it in the name is redundant and 
too long.  Sorry for caring maybe overly about the names. That's how I 
find things.

thanks,
Coleen
>
>> I'll read the thread.  We've had a runtime offsite all week and haven't been able to read this thread, and thought you'd checked in what I'd reviewed before.
>>
> There is no change in the hotspot part.
>
> Mandy
> [1] https://bugs.openjdk.java.net/browse/JDK-8150778
>
>