Review request on the optional modules support

[mark.reinhold at oracle.com]

2011/8/4 12:35 -0700, mandy.chung at oracle.com:
> An update on the optional module support [1].
> 
> Webrev:
>    http://cr.openjdk.java.net/~mchung/jigsaw/webrevs/optional-modules.00/

I looked at the newer version, as you suggested:

    http://cr.openjdk.java.net/~mchung/jigsaw/webrevs/optional-modules.01/

> The note has been updated to include the open questions raised in
> the discussion.
>    http://openjdk.java.net/projects/jigsaw/doc/topics/optional.html
> 
> ...

This mostly looks good but I have a few suggestions and questions.

Line numbers are in [brackets].

org/openjdk/jigsaw/Context.java

  [158] Why a TreeSet?  Is order important?  If not, use a HashSet.

  [166] s/reexporting/re-exporting/

org/openjdk/jigsaw/Loader.java

  [131..132] s/mname/mn/g

  [158..164] Suggest reversing the order of these two statements.  If you
  find the library first then you can pass that to the findModule method,
  avoiding a second lookup in that method.

  [182] s/kernel/boot/

  [189] Brace should be at end of previous line.

  [190] s/this class's/this/

  [196] If m != null then you can just return it here and remove the test
  on [204].

  [382..388] The isModulePresent method should be defined earlier, before
  toString() [256].

org/openjdk/jigsaw/BootLoader.java

  [55] BASE_MODULE should be defined in Platform.java; it doesn't belong
  here.

  [58] If you're going to provide a factory method to enforce a singleton
  instance [118] then declare the constructor to be private.

  [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.

  [131] Brace should be at end of previous line.

org/openjdk/jigsaw/Linker.java

  [246] This comment should now mention that the supplier-name maps are
  being updated too.

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).

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?

  [47] If you rename this to "value" then annotations of the form

      @RequireOptionalModule(modules={"jdk.jaxp"})

  can be shortened to

      @RequireOptionalModule("jdk.jaxp")

- Mark