> On Oct 19, 2016, at 11:34 AM, Steve Drach <[email protected]> wrote:
>
> 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.
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.
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);
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.
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.
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.
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).
92 private int jar(String cmd) {
Nit: this can simply take a varargs and no need to split:
jar(String… options)
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.
Mandy
