8170987: Module system implementation refresh (12/2016)

[Chris Hegarty]

> On 15 Dec 2016, at 01:17, Mandy Chung <mandy.chung at oracle.com> wrote:
> ...
> 
> src/java.base/share/classes/jdk/internal/module/ModuleResolution.java
> 
>  64             throw new RuntimeException("cannot add deprecated to " + value);
> 
> This comment applies to ModuleResoluton::with* methods.  This should
> probably be an InternalError?  

I think InternalError is suitable here. These checks are to ensure tools don’t do
anything inappropriate.

> 108         return String.valueOf(value);
> 
> Nit: since you override toString method, might be helpful to print
> an informative description.  

Done.

> src/jdk.jlink/share/classes/jdk/tools/jmod/JmodTask.java
> 
> 1102             if (value.equals("deprecated"))
> 1103                 return (new ModuleResolution(0)).withDeprecated();
> 1104             else if (value.equals("deprecated-for-removal"))
> 1105                 return (new ModuleResolution(0)).withDeprecatedForRemoval();
> 1106             else if (value.equals("incubating"))
> 1107                 return (new ModuleResolution(0)).withIncubating();
> 
> Why not passing the flag to ModuleResolution constructor?  Similar
> statement is also in sun/tools/jar/GNUStyleOptions.java.

I cleaned this up a little. I don’t want the tools to have knowledge of the actual 
flag values.

> I was wondering if jmod describe and jar —-print-module-descriptor should
> print all optional attributes.  While the module resolution is of limited
> use, it would be handy to print all optional attributes, if present rather
> than having to run java.

Agreed.

> It’s okay to follow up as a separate JBS issue if we want to do that.

If I don’t get to it before this Friday, I’ll follow up with a separate issue.

> test/jdk/modules/incubator/ImageModules.java
>  @modules jdk.jlink jdk.jartool are missing.  I have fixed it.

Thanks.
-Chris.