On Tue, 29 Apr 2025 10:12:07 GMT, Maurizio Cimadamore <mcimadam...@openjdk.org> wrote:
>> Jan Lahoda has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains 13 commits: >> >> - Merge branch 'master' into JDK-8344708 >> - Adjusting ToolProviderTest to the updated persistence handling. >> - Merge branch 'master' into JDK-8344708 >> - Reflecting review feedback: cleanup formatting in ModuleInfo. >> - Reflecting review feedback - avoiding hardcoded constants. >> - Fixing test. >> - Cleanup, fixing tests. >> - Adjusting JShell defaults. >> - Fixing tests. >> - Cleanup - updated specification will permit requires transitive >> java.base; for all classfile versions; java.se no longer participates in >> preview. >> - ... and 3 more: https://git.openjdk.org/jdk/compare/1f228e55...da39d0ef > > src/jdk.jshell/share/classes/jdk/internal/jshell/tool/Startup.java line 345: > >> 343: boolean hasModuleImports = source == null || >> 344: >> Feature.MODULE_IMPORTS.allowedInSource(source); >> 345: int idx = preview ? 2 : !hasModuleImports ? 1 : 0; > > It feels like this would be cleaner with an if/else if/else? I'm also not a > big fan of hardcoded constants into an array. Maybe this code can benefit > from stable values (when they are integrated). (in the interim, maybe using an enum with the possible values, instead of indices, and then turning `defaultStartup` into an EnumMap might make the code slightly clearer?) ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/23801#discussion_r2065993361
