> On 15 Dec 2016, at 01:17, Mandy Chung <mandy.ch...@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.
