RFR [9] 8137058: Clear out all non-Critical APIs from sun.reflect and move to jdk.unsupported

[Chris Hegarty]

> On 13 Apr 2016, at 19:03, Paul Benedict <pbenedict at apache.org> wrote:
> 
> Since getCallerClass will be removed in 10, @Deprecated should also have its condemned=true


`condemned` was renamed to `forRemoval` [1].  getCallerClass, in fact the
whole class, will have `forRemoval=true`. 

-Chris.

[1] http://mail.openjdk.java.net/pipermail/jdk9-dev/2016-April/003932.html

> 
> Cheers,
> Paul
> 
> On Wed, Apr 13, 2016 at 12:43 PM, Mandy Chung <mandy.chung at oracle.com <mailto:mandy.chung at oracle.com>> wrote:
> 
> > On Apr 13, 2016, at 10:28 AM, Chris Hegarty <chris.hegarty at oracle.com <mailto:chris.hegarty at oracle.com>> wrote:
> >
> > Mandy,
> >
> > On 13/04/16 18:15, Mandy Chung wrote:
> >>
> >>> On Apr 13, 2016, at 8:43 AM, Chris Hegarty <chris.hegarty at oracle.com <mailto:chris.hegarty at 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/ <http://cr.openjdk.java.net/~chegar/8137058/>
> >>> https://bugs.openjdk.java.net/browse/JDK-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
>