> On 13 Apr 2016, at 19:03, Paul Benedict <pbened...@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.ch...@oracle.com > <mailto:mandy.ch...@oracle.com>> wrote: > > > On Apr 13, 2016, at 10:28 AM, Chris Hegarty <chris.hega...@oracle.com > > <mailto: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 > >>> <mailto: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/ > >>> <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 >