On Tue, 28 Nov 2023 21:46:58 GMT, Mandy Chung <mch...@openjdk.org> wrote:
>> Severin Gehwolf has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Tighten ModifiedFilesExitTest
>>
>> Ensure the error message is reasonable and doesn't include
>> Exceptions presented to the user.
>
> src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/AddRunImageResourcesPlugin.java
> line 67:
>
>> 65: // RunImageArchive for further processing.
>> 66: private static final String RESPATH = RESPATH_PREFIX +
>> "%s_resources";
>> 67: private static final String JLINK_MOD_NAME = "jdk.jlink";
>
> There are 2 other occurrences of "jdk.jlink". I guess you plan to replace
> them with the constant variable?
I think I've replaced them in the latest update.
> src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/AddRunImageResourcesPlugin.java
> line 116:
>
>> 114: if (platform == null) {
>> 115: throw new IllegalStateException("java.base not part of the
>> image?");
>> 116: }
>
> Can simply use `orElseThrow`. It should not reach here and so it's ok to use
> InternalError or AssertionError (which is also used in this code).
>
> Suggestion:
>
> String platform = in.moduleView().findModule("java.base")
> .map(ResourcePoolModule::targetPlatform)
> .orElseThrow(() -> new AssertionError("java.base not found"));
Done.
> src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/AddRunImageResourcesPlugin.java
> line 121:
>
>> 119:
>> 120: private void addModuleResourceEntries(ResourcePoolBuilder out) {
>> 121: for (String module: keysInSortedOrder()) {
>
> Suggestion:
>
> nonClassResEntries.keySet().stream().sorted().forEach(module ->
> {
>
>
> `keysInSortedOrder` can be removed.
removed.
> src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/AddRunImageResourcesPlugin.java
> line 126:
>
>> 124: if (mResources == null) {
>> 125: throw new AssertionError("Module listed, but no
>> resources?");
>> 126: }
>
> Since it is always non-null, this check can be dropped. NPE will be thrown
> if such bug exists.
I've replaced this with `Object.requireNonNull()`.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1414357943
PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1414358486
PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1414359291
PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1414359118
