On Tue, 15 Oct 2024 09:52:27 GMT, Serguei Spitsyn <sspit...@openjdk.org> wrote:
>> Alex Menkov has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> renamed JVM_EnqueueOperation2
>>
>> renamed JVM_EnqueueOperation2 to JVM_EnqueueOperation_v2 for consistency
>> (per Serguei request)
>
> src/hotspot/os/windows/attachListener_windows.cpp line 40:
>
>> 38: // executes a small stub generated by the client. The stub invokes the
>> 39: // JVM_EnqueueOperation or JVM_EnqueueOperation_v2 function which checks
>> the operation parameters
>> 40: // and enqueues the operation request to the queue serviced by the
>> attach listener. The thread created by
>
> Nit: Just a suggestion to simplify this line of comment a little bit:
> // and enqueues the operation request for the attach listener. The thread
> created by
I've made it "enqueues the operation request to the queue."
> src/hotspot/os/windows/attachListener_windows.cpp line 108:
>
>> 106: size,
>> 107: &nread,
>> 108: nullptr); // not overlapped
>
> Nit: Missed space after `fsuccess` at line 104.
> Also, there is not cast `(DWORD)size` at line 106 as it is done in the
> `WriteFile()` call at line 118.
Fixed both indent and cast
> src/hotspot/os/windows/attachListener_windows.cpp line 194:
>
>> 192: }
>> 193: const char* arg(int i) const {
>> 194: return (i >= 0 && AttachOperation::arg_count_max) ? _arg[i] :
>> nullptr;
>
> Nit: Dot is not needed at the end of comment at line 140.
> Q: Should the condition at line 194 be modified as below:
> ```return (i >= 0 && i < AttachOperation::arg_count_max) ? _arg[i] :
> nullptr;```
Good catch! Fixed
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/20782#discussion_r1802084434
PR Review Comment: https://git.openjdk.org/jdk/pull/20782#discussion_r1802084837
PR Review Comment: https://git.openjdk.org/jdk/pull/20782#discussion_r1802086150
