On Fri, 7 Feb 2025 18:53:40 GMT, Justin Lu <j...@openjdk.org> wrote:
>> Naoto Sato has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Reflects review
>
> 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.
Thanks. I modified the comment.
> 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.
> 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.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/23498#discussion_r1947128400
PR Review Comment: https://git.openjdk.org/jdk/pull/23498#discussion_r1947128384
PR Review Comment: https://git.openjdk.org/jdk/pull/23498#discussion_r1947128439
