On Wed, 16 Apr 2025 14:43:45 GMT, Roger Riggs <rri...@openjdk.org> wrote:
>> Stuart Marks 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 four additional >> commits since the last revision: >> >> - Move asserts into platform-independent code. >> - Merge branch 'master' into JDK-8354464-additional-cleanup-native-encoding >> - Rename variables to make clear they're not used. >> - 8354464: Additional cleanup setting up native.encoding > > src/java.base/windows/native/libjava/java_props_md.c line 663: > >> 661: &sprops.format_country, >> 662: &sprops.format_variant, >> 663: &format_encoding_unused); > > A cleaner change would be to modify SetupI18nProps to remove the encoding > argument. > SetupI18nProps is a file local static function, there are no other uses. > And drop the dummy variables. The main focus of this commit is to straighten out the logic between the platform-specific and platform-independent layers. In the previous state of this file, tracing the origin of the properties on Windows, I ended up following the origin of sprops.encoding all the way through SetupI18nProps, only to find that was ignored entirely at the platform-independent layer. So this change is an improvement in that it clarifies where sprops.encoding comes from, and that SetupI18nProps does some computations that end up being thrown away instead of being stored into a shared data structure that's eventually ignored. Of course that path is dead code and could be removed without changing the behavior. But this code has a tortured history and it's not entirely clear that it's correct in its current state. In fact @naotoj considered making some changes in this logic [JDK-8352917](https://bugs.openjdk.org/browse/JDK-8352917) but decided against it. However it seems like a possibility that we might revisit this? Also note that we're now talking about how Windows gets its encoding information from the OS, and not how encoding information is transmitted up through the layers into Java properties, which is the focus of this cleanup. Anyway I'd like to hear from @naotoj on this. If we're certain the extra stuff in SetupI18nProps is useless, we should clean it up now. But if there might be a use for this information -- maybe stdin.encoding? -- then perhaps it's better to leave it as is. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/24607#discussion_r2047428294
