On 14/12/2016 23:31, Claes Redestad wrote:
Hi,
I took a quick pass over the jdk changes. It generally looks very good,
but I've got some comments:
MethodHandles.Lookup.dropLookupMode: The javadoc doesn't really roll
of the tongue here. Maybe "Creates a new lookup from the current one
where the given lookup mode has been dropped. ..." for starters?
John has a few tweaks to javadoc and also changes it to allow PROTECTED
be dropped. I will get those changes into jake today and then see if we
can improve the wording a bit.
ModuleDescriptor$Builder: should automatic be moved into a constructor
and automatic(boolean) removed for consistency with other boolean
attributes? My gut feeling tells me that
Builder.module("name").automatic(true) is non-sensical (not to mention
Builder.automaticModule("name").automatic(false)). It probably makes
no sense to export it through the JLMA bridge, but could avoid that
by adding a new private constructor called by the current.
We have re-visit a few things here as there are open questions on
whether creating an automatic module via the Builder should require all
packages to be exported and open. So I expect there will be changes here
once we get to that overhaul.
WARNING could be a local anonymous class inside
printStackTraceIfExposedReflectively. ;-) A more noticeable cleanup
would be to move these methods to jdk/internal/reflect/Reflection.java
where there's now what appears to be code duplication (although the
printed messages diverge).
I'll look at it again, it was done this way to make it easy to leave the
DEBUG_ADD_OPENS out.
In ClassWriter.java there's a comment line that seems to have been
removed by mistake.
Ugh, well spotted.
-Alan
