On Fri, 7 Feb 2025 20:11:06 GMT, Naoto Sato <na...@openjdk.org> wrote:
>> 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 think the overhead is negligible. If we use the Get... function, we will
> need to allocate/deallocate the intermediate buffer, which will make the code
> complex.
OK, seems good to use `_wgetenv` then.
>> 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.
>
> I am not sure of this. Isn't it the issue in jtreg plugin? At least `make
> test TEST=...` will succeed.
Oops, I thought the test would mark as "skipped" or something like that, but
yes its a Windows only test, hence why it does not show up, duh.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/23498#discussion_r1947288085
PR Review Comment: https://git.openjdk.org/jdk/pull/23498#discussion_r1947288095
