On Mon, 22 Nov 2021 08:25:38 GMT, Alan Bateman <[email protected]> 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
