>> issue: https://bugs.openjdk.java.net/browse/JDK-8164805
>> webrev: http://cr.openjdk.java.net/~sdrach/8164805/webrev.00/
>
> Issue a warning is good in the case public classes are added in a concealed
> package. Some comments:
>
> Main.java
>
> addExtendedModuleAttributes does not seem to be the appropriate method to
> initialize concealedPackages. It would be better to set concealedPackages in
> a separate method that will be called for each modular JAR.
I made a simple change to existing code, I wasn’t intending to make significant
changes to jar tool. Of course as time goes on, jar tool continues to grow
into a bigger hair ball. It would definitely benefit from major cosmetic
surgery. Perhaps I don’t understand the point you are trying to make.
>
> Validator.java
> 160
> main.error(Main.formatMsg("error.validator.concealed.public.class",
> entryName));
>
> This is a warning. You should add a new warn method to make it clear.
Ok.
> Similiarly, “error.validator.concealed.public.class” should be renamed to
> “warn.validator.concealed.public.class”. I notice there are several keys
> with “error.validator” prefix that are meant for warnings. It’d be good to
> change them too.
>
> 247 if (idx > -1) {
> 248 String pkgName = internalName.substring(0, idx).replace('/',
> '.');
> 249 if (main.concealedPackages.contains(pkgName)) {
> 250 return true;
> 251 }
> 252 }
>
> Alternative impl for the above lines:
>
> String pkgName = idx != -1 ? internalName.substring(0, idx).replace('/', '.’)
> : “”;
> return main.concealedPackages.contains(pkgName);
Ok.
>
> jar.properties
>
> 112 error.validator.concealed.public.class=\
> 113 warning - entry {0} is a public class in a concealed package,
> placing this \
> 114 jar on the class path will result in incompatible public
> interfaces
>
> line 113 needs new line ‘\n’. You may break it into 3 lines
> since the entry name will take up some characters.
Ok.
>
> ConcealedPackage.java test
>
> 60 Path destination = userdir.resolve("classes”);
>
> I suggest to use Paths.get(“classes”) rather than ${user.dir}.
Is there a performance advantage or some other reason for doing this? The way
I do it seems reasonable.
> jtreg will clean up the JTwork/scratch directory after the test run.
That’s what the docs say but I’ve seen problems in the past with windows
machines, so I just got in the habit
of doing my own clean up.
>
> 63 // add an empty directory
> 64
> Files.createDirectory(destination.resolve("p").resolve("internal"));
>
> I suggest to take this out. The test verifies if "jar tf mmr.jar” succeeds.
Ok. Just trying to make it exactly the same as the jar structure in the bug
report.
>
> The test should test both “cf” and “uf” and verify that the ConcealedPackage
> attributes in module-info.class is updated correctly (one single
> module-info.class case in the root entry and two module-info.class - root and
> versioned).
Ok.
>
> 92 private int jar(String cmd) {
> Nit: this can simply take a varargs and no need to split:
> jar(String… options)
I like the String because it’s more readable and I suspect the split isn’t that
costly.
>
>
> 76 private String files(Path start) throws IOException {
> Perhaps return Stream<String>. That can replace the unnecessary concat in a
> String and then later split to pass to javac.
What we probably want is stream.toArray(String[]::new), but then we’d have to
copy that array into a new array that is 2 elements larger to accommodate the
destination parameter. I wonder if that’s any better. I need to think about
it.
>
> Mandy
