8170987: Module system implementation refresh (12/2016)

[Claes Redestad]

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?

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.

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 see the Checks.isJavaIdentifier has been reworked, which should also
resolve the correctness issues we found here[1]. Good!

In ClassWriter.java there's a comment line that seems to have been
removed by mistake.

Thanks!

/Claes

[1] https://bugs.openjdk.java.net/browse/JDK-8170601

On 2016-12-14 22:46, Alan Bateman wrote:
> Folks on jigsaw-dev will be aware that we are on yet another mission to
> bring the changes accumulated in the jake forest to jdk9/dev. The plan
> this time is to bring the changes to jdk9/dev to make jdk-9+150.
>
> The changes in this update are mostly for JSR 376 issues
> #VersionedDependences and #ModuleNameCharacters and so involve updates
> to the binary form of the module declaration. There is also some small
> changes left over from #IndirectQualifiedReflectiveAccess that we didn't
> include in the last refresh.
>
> This update has the implementation of Incubator Modules (JEP 11 [1]),
> everything except the javac support. This was initially planned to push
> to jdk9/dev but was re-routed to jake to avoid needing re-work when
> merged with the changes in jake.
>
> There is a bit of refactoring in the implementation in this update. We
> expect to do more on than, plus lots of clean-up, once all the feature
> work is out of way.
>
> The webrevs with the changes for this update are here:
>
>    http://cr.openjdk.java.net/~alanb/8170987/1
>
> They are currently based on jdk-9+148 and will be re-based for jdk9/dev
> later this week.
>
> One review note this time is to ignore the changes in ModuleBootstrap
> for DEBUG_ADD_OPENS, that is the only change in this webrev that is not
> proposed to move to jdk9/dev.
>
> -Alan
>
> [1] http://openjdk.java.net/jeps/11
>