8170987: Module system implementation refresh (12/2016)

[Alan Bateman]

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