> On Apr 13, 2016, at 10:28 AM, Chris Hegarty <chris.hega...@oracle.com> wrote:
>
> Mandy,
>
> On 13/04/16 18:15, Mandy Chung wrote:
>>
>>> On Apr 13, 2016, at 8:43 AM, Chris Hegarty <chris.hega...@oracle.com> wrote:
>>>
>>> This review is for the second significant part of the changes for JEP
>>> 260 [1]. When created, the jdk.unsupported module [2] initially
>>> contained the sun.misc package. This issue is will move all the
>>> non-Critical APIs out of sun.reflect, leaving only the critical ones, at
>>> which point sun.reflect can be moved to the jdk.unsupported module.
>>>
>>> http://cr.openjdk.java.net/~chegar/8137058/
>>> https://bugs.openjdk.java.net/browse/JDK-8137058
>>>
>>> Summary of the changes:
>>>
>>> - Move all existing sun.reflect types to jdk.internal.reflect, and
>>> fix up references in the libraries, and the VM, to use the new internal
>>> location.
>>
>> I hope the getCallerClass(int depth) method is moved out of
>> jdk.internal.reflect.Reflection. JDK is not using it and it’s retained for
>> existing libraries to migrate to the new StackWalker API. Of course the
>> cost is to add a native library and the build infra work has made this
>> straight-forward.
>
> In my patch jdk.internal.reflect.Reflection.getCallerClass(int)
> is retained only to avoid the need for an unsupported.so, but
> if you feel strongly that is should go, then I can make the
> change.
I’m less concerned of a native library but its name led me to have a second
thought. I can leave with keeping
jdk.internal.reflect.Reflection::getCallerClass(int depth) for another reason.
>
>> I agree with Alan that the @deprecated javadoc of
>> sun.reflect.Reflection::getCallerClass should be updated to make it clear.
>> What about:
>>
>> /**
>> * @deprecated This method is an internal API and will be removed in JDK 10.
>> * Use {@link StackWalker} to walk the stack and obtain the caller class
>> * with {@link StackWalker.StackFrame#getDeclaringClass} instead.
>> */
>
> That seems reasonable. The version number should be present
> in the @Deprecated forRemoval element too. I'll make the change.
Thanks.
>
>> This patch will likely impact existing libraries that filter out reflection
>> frames (IIRC Groovy and log4j may be examples) doing
>> Class::getName().startsWith(“sun.reflect”). It may worth call out this
>> incompatibility in JEP 260.
>
> I'll add a note.
>
>> StackStreamFactory.java shows an example:
>>
>> 1085 if (c.getName().startsWith("sun.reflect") &&
>>
>> This should be changed to “jdk.internal.reflect”.
>
> Ah I missed this. Strangely all tests are passing without
> this change. I'll make the change.
This is just like an assertion that should never reach there. It throws an
internal error if a class starts with sun.reflect but not a reflective method.
>
>> test/java/lang/StackWalker/EmbeddedStackWalkTest.java and maybe a few other
>> stack walker tests. You may want to check any other of any similar pattern
>> in the JDK code/tests (I think I got them all)
>
> I did update some StackWalker tests, but missed this one ( it
> passes for me ). I'll check all of them.
It may be a check with several conditions or assertion like the above.
Mandy
