On Mon, 22 Nov 2021 08:25:38 GMT, Alan Bateman <al...@openjdk.org> wrote:
>> Jaikiran Pai has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains seven additional >> commits since the last revision: >> >> - Merge latest from master branch >> - use testng asserts >> - Remove "final" usage from test >> - convert test to testng >> - Merge latest from master branch >> - Merge latest from master branch >> - 8258117: jar tool sets the time stamp of module-info.class entries to the >> current time > > src/jdk.jartool/share/classes/sun/tools/jar/Main.java line 806: > >> 804: Long lastModified = f.lastModified() == 0 ? null : >> f.lastModified(); >> 805: moduleInfos.putIfAbsent(name, >> 806: new StreamedModuleInfoEntry(name, >> Files.readAllBytes(f.toPath()), lastModified)); > > I'd prefer to see this split into two two statements as there is just too > much going on one the one line. Done. I have updated the PR to split this line into more than 1 statement. > src/jdk.jartool/share/classes/sun/tools/jar/Main.java line 1785: > >> 1783: private final String name; >> 1784: private final byte[] bytes; >> 1785: private final Long lastModifiedTime; > > It might be better to use a FileTime here, otherwise we risk a NPE when > unboxing. Sounds good. I've updated the PR to replace this to use `FileTime`. > src/jdk.jartool/share/classes/sun/tools/jar/Main.java line 2108: > >> 2106: byte[] extended = extendedInfoBytes(md, bytes, packages); >> 2107: // replace the entry value with the extended bytes >> 2108: e.setValue(new >> StreamedModuleInfoEntry(e.getValue().name(), extended, >> e.getValue().getLastModifiedTime())); > > There are 3 calls to getValue and each one I need to remember that e.getValue > is a ModuleInfoEntry. If you add ModuleInfo entry = e.getValue() then it > might help the readability. That makes sense. The updated PR now has this change. > test/jdk/tools/jar/modularJar/JarToolModuleDescriptorReproducibilityTest.java > line 60: > >> 58: private static final String UPDATED_MODULE_VERSION = "1.2.4"; >> 59: private static final String MAIN_CLASS = "jdk.test.foo.Foo"; >> 60: private static final Path MODULE_CLASSES_DIR = >> Paths.get("8258117-module-classes", MODULE_NAME).toAbsolutePath(); > > You can use Path.of here. Done. Replaced this and one other place in this test to use `Path.of`. ------------- PR: https://git.openjdk.java.net/jdk/pull/5486