There is a new webrev at http://cr.openjdk.java.net/~sdrach/8164805/webrev.01/ <http://cr.openjdk.java.net/~sdrach/8164805/webrev.01/> that addresses the issues raised by Mandy. Comments inline.
>> 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. Done, although I still don’t see an advantage to this. > > 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. > 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. All done. > > 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); Done. > > 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. Done. > > ConcealedPackage.java test > > 60 Path destination = userdir.resolve("classes”); > > I suggest to use Paths.get(“classes”) rather than ${user.dir}. jtreg will > clean up the JTwork/scratch directory after the test run. Doen, although I still clean up after tests. > > 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. Done. > > 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). Done, but perhaps not exactly the suggested way. The test has been considerably beefed up. > > 92 private int jar(String cmd) { > Nit: this can simply take a varargs and no need to split: > jar(String… options) I left it the way it was. > > > 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. Done. Also fixed a bug I found Main::toPackageName
