On Wed, 11 Dec 2024 21:11:39 GMT, Mandy Chung <mch...@openjdk.org> wrote:
>> Severin Gehwolf has updated the pull request incrementally with three
>> additional commits since the last revision:
>>
>> - Remove too strong assertion
>> - Test fixes
>> - Address comments from Mandy
>
> src/jdk.jlink/share/classes/jdk/tools/jlink/internal/JlinkTask.java line 500:
>
>> 498: * {@code roots} set.
>> 499: */
>> 500: public static ModuleFinder limitFinder(ModuleFinder finder,
>
> Inlining `limitFinder` sounds good while I suggest keeping the method name as
> `newModuleFinder` which is clear.
>
> Suggestion:
>
> public static ModuleFinder newModuleFinder(ModuleFinder finder,
Personally, I find it less clear to call it `newModuleFinder`. What it does is
that it creates a finder based on the passed in finder that "finds" fewer
modules. How about `newLimitedFinder()`? But no strong feelings, so if you
think it should be `newModuleFinder` then it's fine by me.
> src/jdk.jlink/share/classes/jdk/tools/jlink/internal/JlinkTask.java line 551:
>
>> 549: * version.
>> 550: */
>> 551: private static void checkVersion(ModuleFinder finder) {
>
> Suggestion:
>
> private static void checkJavaBaseVersion(ModuleFinder finder) {
Sure.
> test/jdk/tools/jlink/IntegrationTest.java line 160:
>
>> 158: JlinkConfiguration config = new Jlink.JlinkConfiguration(output,
>> 159: mods,
>> 160:
>> JlinkTask.limitFinder(JlinkTask.newModuleFinder(modulePaths), limits, mods),
>> linkFromRuntime, false, false);
>
> `linkFromRuntime` parameter is no longer needed and this should fail
> compilation.
The parameter is also passed to `JlinkConfiguration` constructor. That still
has the parameter.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/22609#discussion_r1881693366
PR Review Comment: https://git.openjdk.org/jdk/pull/22609#discussion_r1881698660
PR Review Comment: https://git.openjdk.org/jdk/pull/22609#discussion_r1881696908
