On 15/05/2018 01:52, David Holmes wrote:
This review is being spread across four groups: langtools, core-libs,
hotspot and serviceability. This is the specific review thread for
core-libs - webrev:
http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v1/
The API additions mostly look good, just a few comments:
- Can the spec for Class::getNestHost be explicit that it returns "this"
when the class is a primitive or array class?
- Can Class::getNestMembers specify if it runs the class initializer of
any classes that it loads? Less of an issue for getNestHost as the host
is likely loaded already.
- I suspect the @throws SecurityException in getNestMembers was copied
from getNestHost as it uses "returned class" (singular). As the host and
members are in the same runtime package then maybe it can be specified
in terms of the host or members package?
- isNestmateOf could be a bit clearer that it returns false in the event
that the attributes can't be parsed. I realize it's specified in terms
of getNestHost but I'm sure you see what I mean.
The tests for core reflection are in test/jdk/java/lang/reflect and I'm
wondering if there has been any discussion about adding at least some
basic tests there? I see the tests in
test/hotspot/jtreg/runtime/Nestmates/reflectionAPI but this isn't an
obvious location for someone touching this code.
The test in InterfaceAccessFlagsTest.java can be disabled with
@Test(enabled=false), might be cleaner than commenting it out.
Maybe a question for Kumar but are we planning to pull in any ASM
updates for JDK 11? NestMembers extends Attribute looks okay, I'm less
sure about the change to ClassReader as I don't know if there is
somewhere else in ASM that has the list of attributes to always parse.
The update to the access check in Reflection.java looks okay (just need
to use 4-space indent, not 2). I think it's okay in VerifyAccess too
but the "FIX ME" suggests there are doubts so I assume this needs more
eyes to triple check.
In passing, you might want to double check the javadoc links. I noticed
one #getNestHost that is missing parenthesis and I'm not sure how
javadoc handles that.
-Alan.
