Hi Alan,
Many thanks for looking at this!
Let me start by saying that generally any spec changes (even
non-normative) have to pass a high bar given the number of levels of
review the spec, ie API, has already had. That isn't to say that no
changes will be considered but I'm very wary of making last minute
changes during code review, without all the parties previously involved
with, and content with, the spec being party to the conversation. Such
changes also affect the CSR request. So my initial position will be that
of the immovable object. ;)
On 15/05/2018 11:53 PM, Alan Bateman wrote:
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?
This is covered by:
"A class or interface that is not explicitly a member of a nest, is a
member of the nest consisting only of itself, and is the nest host.
Every class and interface is a member of exactly one nest."
Do we really need to spell out the case for primitives and arrays? If so
would it suffice to add the following:
"A class or interface that is not explicitly a member of a nest *(such
as primitive or array classes)*, is a member of the nest consisting only
of itself, and is the nest host. Every class and interface is a member
of exactly one nest."
- 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.
None of these methods specifically or intentionally initialize any
classes. A class will be loaded if needed but that is all. The only
Class methods that refer to class initialization are the forName()
variants. I don't see that the nest related methods should do any more
or less in this regard than any other Class methods that may require
classes to be loaded.
- I suspect the @throws SecurityException in getNestMembers was copied
from getNestHost as it uses "returned class" (singular).
It refers to "If any returned class ..." and "that returned class". I
don't see any problematic singular uses - can you elaborate please.
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?
I'm not sure how to accurately formulate that. The current wording was
based on similar @throws in getEnclosingClass, as suggested by Mandy:
http://mail.openjdk.java.net/pipermail/valhalla-dev/2018-March/003955.html
and then refined a little.
- 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.
Just to be clear, parsing of nest attributes occurs at class load time
and must be syntactically correct else the class won't load. Validation
of those attributes (i.e agreement between the purported nest-host class
and the claimed nest-member class) is what is deferred until either an
access check is needed, or these new Class reflection methods are
called. So when getNestHost is called the current class is obviously
already loaded, but the purported nest-host may need to be loaded which
may encounter:
- linkage-errors (NoClassDefFoundError, VerifyError,
ClassfileFormatError etc)
- runtime errors (OOME, stackoverflow)
- disagreement between the nest-host and this class about nest membership
I just noticed that there is a wording error in getNestHost() as it states:
"If there is any error accessing the nest host, ... then this is returned."
but in the implementation that's only true for LinkageErrors. Runtime
errors get propagated. I think this is what we want (other places in MH
runtime where runtime-errors were getting swallowed or converted have
since been switched back to propagate them.) But I'll need to run this
past the EG to check.
That notwithstanding, exactly what would you propose here? The current
spec is clear and concise and precise, but defers to getNestHost
somewhat for details on what happens when things go wrong.
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.
Good catch! It was always my intent to redistribute the new nestmate
tests into their component areas where applicable and appropriate - but
I forgot to do that before calling for the RFR. I will move them to
java/lang/reflect/Nestmates.
The test in InterfaceAccessFlagsTest.java can be disabled with
@Test(enabled=false), might be cleaner than commenting it out.
Thanks - will fix.
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.
Remi already responded on this. Seems what we have now is okay but
eventually it will have to be reconciled when we update from upstream ASM.
The update to the access check in Reflection.java looks okay (just need
to use 4-space indent, not 2).
Will fix the indent - thanks for spotting.
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.
The "FIX ME" is not about the access check but the form of the
assertion. The assert is verification that resolution of a private
method always leads to selection of that private method: refc == defc. I
used "myassert" so that this check was always enabled during testing.
The "FIX ME" was to either convert to language assert statement or else
remove it (having validated that it never fires). Obviously "myassert"
has not fired in all the testing that I have done so either choice seems
fine. Do you have a preference?
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.
Should have generated a warning so I'll fix that - thanks.
Thanks again for the review.
David
-Alan.
