Since getCallerClass will be removed in 10, @Deprecated should also have its condemned=true
Cheers, Paul On Wed, Apr 13, 2016 at 12:43 PM, Mandy Chung <mandy.ch...@oracle.com> wrote: > > > 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
