Review request on the optional modules support

[Mandy Chung]

On 9/7/11 2:58 PM, mark.reinhold at oracle.com wrote:
> This mostly looks good but I have a few suggestions and questions.
Thanks for the review.  I'll make the change per your suggestions.
My comments inlined below.

> Line numbers are in [brackets].
>
> org/openjdk/jigsaw/Context.java
>
>    [158] Why a TreeSet?  Is order important?  If not, use a HashSet.
Order is not important and will change it to HashSet.


>
>
> org/openjdk/jigsaw/BootLoader.java
>
>
>    [94..115] I don't understand why this override is necessary.  The boot
>    and base modules will be in the same context, so the definition in the
>    Loader superclass should work correctly.

The base and boot modules were made to be in different contexts to
implement the exports work [1]. In the prototype I built for exports,
if they are in the same context, the internal types exported from the
boot module will be visible to the applications requiring jdk.base.
As we have to explore further what the best model for exports,
I should keep them in the same context in this patch.

I do think this override is not necessary if they are in the same
context.  But I'm afraid split package could be one potential issue.
I'll make the change and double check to confirm.

One issue I ran into is that:

java.util.XMLUtils is currently in sun.jaxp module due to
its dependency on jaxp API.  java.util package is split
between jdk.boot and sun.jaxp and thus they are in the same
context.  In this case, jdk.base can't require optional jdk.jaxp.
Otherwise it will fail to install with:

    org.openjdk.jigsaw.ConfigurationException: Package javax.crypto.interfaces
    defined in +jdk.base+jdk.boot+sun.charsets+sun.jaxp but exported by
    supplier +jdk.jaxp

This is not a good example since java.util.XMLUtils should be renamed
to another package to avoid the split package.  It's on the todo list
and I'll do the rename in this fix and see if there is any other issue.

>
> java/lang/reflect/Module.java
>
>    I think the isModulePresent and requireModulePresent methods really
>    belong in java.lang.module.ModuleClassLoader.  These methods query
>    a module context, not a module, and a ModuleClassLoader holds the
>    run-time representation of a module context.
>
>    This makes the usage idioms a little clunkier since now you have to
>    cast the result of Class.getClassLoader():
>
>        ((ModuleClassLoader)this.class.getClassLoader()).isModulePresent(mn)
>
>    but that could be ameliorated by defining a convenience method in
>    java.lang.Class which does the cast for you (and maybe returns the boot
>    module loader rather than null when in legacy mode, as we've discussed
>    previously).

I also considered defining these methods in ModuleClassLoader
and add a convenience method in java.lang.Class that would simplify
the call to these methods after posting the webrev.  I thought I added
that question to in optional.html to look for feedback but apparently
it's still in my private copy :(

That's good and I'll make the change.

>
> java/lang/module/RequireOptionalModule.java
>
>    Shouldn't this be named "RequiresOptionalModule", to match the fact
>    that we use "requires" rather than "require" in module declarations?

I thought the new proposed grammar [2] is "require" rather than "requires".
The current prototype is yet to be updated to implement the new proposal
and thus we still use "requires".


>    [47] If you rename this to "value" then annotations of the form
>
>        @RequireOptionalModule(modules={"jdk.jaxp"})
>
>    can be shortened to
>
>        @RequireOptionalModule("jdk.jaxp")

That looks more concise.

Thanks
Mandy

[1] http://mail.openjdk.java.net/pipermail/jigsaw-dev/2011-June/001333.html
[2] http://openjdk.java.net/projects/jigsaw/doc/topics/grammar.html