Hi,
Chris Hegarty 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 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.
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.
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.
The hotspot change drops the package name prefix in javaClasses.hpp
which is inconsistent with other classes. I found including
jdk_internal_reflect_ in the class name is useful. Coleen and
others from the hotspot team may have opinion on this.
Ok.
FX will need to adjust for this patch (cc’ing Kevin to prepare for this)
Ah ok, thanks for copying Kevin.
Yes, thanks Mandy. JavaFX uses sun.reflect.CallerSensitive and
sun.reflect.Reflection (both in FXML) so I will file an FX bug to track
the need to fix the module dependences.
-- Kevin
I'll send a note to the list once the webrev is updated.
-Chris.
