On Fri, 7 Feb 2025 13:41:03 GMT, Jaikiran Pai <[email protected]> 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 472:
>
>> 470: JLI_AddArgsFromEnvVar(JLI_List args, const char *var_name) {
>> 471: #ifdef _WIN32
>> 472: char *env = winGetEnv(var_name);
>
> I see that we are limiting this call to use `_wgetenv()` (the wide-character
> version of `getenv()`) only to `_WIN32`. Is there any harm in using
> `_wgetenv()` even for `_WIN64`? Or is the `getenv()` implementation on
> `_WIN64` not susceptible to the issues noted in the JBS?
The macro name is confusing, but `_WIN32` means Win32 API set and does not
specify the target platform. This link
(https://learn.microsoft.com/en-us/cpp/preprocessor/predefined-macros?view=msvc-170)
reads
_WIN32 Defined as 1 when the compilation target is 32-bit ARM, 64-bit ARM, x86,
or x64. Otherwise, undefined.
So I believen `_WIN32` is correct here.
> src/java.base/share/native/libjli/args.c line 479:
>
>> 477: jboolean ret = JNI_FALSE;
>> 478:
>> 479: if (firstAppArgIndex != 0 && // If not 'java', return
>
> Hello Naoto, this is a pre-existing thing, but since we are changing this
> part of the code, I think the checks for `firstAppArgIndex` and `relaunch`
> can be moved before the call to `getenv()` (and now before `winGetEnv()` too)
> since the syscall to `getenv()` wasn't anyway changing the state of
> `firstAppArgIndex` or `relaunch`. I think doing those checks before the call
> to fetch the env value will simplify the conditional check here a bit, in
> terms of readability.
Thanks Jai. That is a good point. Moved those checks before winGetEnv().
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/23498#discussion_r1946998326
PR Review Comment: https://git.openjdk.org/jdk/pull/23498#discussion_r1946998307
