On Fri, 7 Feb 2025 18:32:26 GMT, Naoto Sato <na...@openjdk.org> wrote:
>> This is a follow-on fix to >> [JDK-8337506](https://bugs.openjdk.org/browse/JDK-8337506). The Java >> launcher can take arguments from "JDK_JAVA_OPTIONS" environment variable, so >> the same processing is needed. Also, obsolete code for Windows 9x has been >> removed. > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > Reflects review Logic to get the UTF-16 string env value, and map back to a code page string looks good to me. All conversions are done with the `WC_NO_BEST_FIT_CHARS` flag, so indeed there should be no best-fit mappings done. All the failure checks on the return values of `MultiByteToWideChar` and `WideCharToMultiByte` look appropriate as well. I left a few comments. src/java.base/share/native/libjli/args.c line 603: > 601: /* > 602: * getenv() without best-fit mapping > 603: */ I think a quick comment that says something along the lines of env variable name: code page -> UTF-16 variable value : UTF-16 -> code page would be helpful to overview what is happening here. src/java.base/share/native/libjli/args.c line 611: > 609: LPWSTR wcVarName = JLI_MemAlloc(wcCount * sizeof(wchar_t)); > 610: if (MultiByteToWideChar(CP_ACP, 0, var_name, -1, wcVarName, > wcCount) != 0) { > 611: LPWSTR wcEnvVar = _wgetenv(wcVarName); Since we are in Windows specific code, would it make sense to call `GetEnvironmentVariableW` over `_wgetenv`? It won't be as concise since we will have to follow the 2-call style of determining the size, then filling the buffer; but I presume it avoids some overhead, since it's directly apart of the Win32 API? I am also guessing it's the convention for Windows native code, although I'm not fully sure. test/jdk/tools/launcher/DisableBestFitMappingTest.java line 32: > 30: * @requires (os.family == "windows") > 31: * @library /test/lib > 32: * @run junit DisableBestFitMappingTest I think it might be best to re-write this test as a non Junit test. Through IntelliJ it's hard to run this test, I presume because of the combination of it being a Junit test and having a `main` method? If I run the 'launcher' suite of tests, this test does not seem to be included. ------------- PR Review: https://git.openjdk.org/jdk/pull/23498#pullrequestreview-2602530119 PR Review Comment: https://git.openjdk.org/jdk/pull/23498#discussion_r1947013548 PR Review Comment: https://git.openjdk.org/jdk/pull/23498#discussion_r1946999296 PR Review Comment: https://git.openjdk.org/jdk/pull/23498#discussion_r1947024510
