On Mon, 18 Dec 2023 10:25:50 GMT, Thomas Stuefe <stu...@openjdk.org> wrote:
>> Joachim Kern has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - trailing whitespace
>> - Following most of Thomas proposals
>
> src/hotspot/os/aix/os_aix.cpp line 30:
>
>> 28: #pragma alloca
>> 29:
>> 30:
>
> please remove whitespace change
Done
> src/hotspot/os/aix/os_aix.cpp line 193:
>
>> 191: // local variables
>> 192:
>> 193:
>
> please remove whitespace change
Done
> src/hotspot/os/aix/porting_aix.cpp line 1097:
>
>> 1095: }
>> 1096:
>> 1097: pthread_mutex_lock(&g_handletable_mutex);
>
> You can make your life a lot easier by defining an RAII object at the start
> of the file:
>
> struct TableLocker {
> TableLocker() { pthread_mutex_lock(&g_handletable_mutex); }
> ~TableLocker() { pthread_mutex_unlock(&g_handletable_mutex); }
> };
>
> and just place this at the beginning of your two functions
>
> TableLocker lock:
> ...
>
>
> no need to manually unlock then, with the danger of missing a return.
Great, thank you. This was one of the things I thought about, but was not sure,
because I did not fully understood the MutexLocker class and the difference
between Monitor and Mutex.
> src/hotspot/os/aix/porting_aix.cpp line 1143:
>
>> 1141: // entry of the array to the place of the entry we want to
>> remove and overwrite it
>> 1142: if (i < g_handletable_used) {
>> 1143: g_handletable[i] = g_handletable[g_handletable_used];
>
> To be super careful, I would zero out at least the handle of the moved item
> like this:
> `g_handletable[g_handletable_used].handle = nullptr`
Done
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/16920#discussion_r1429950832
PR Review Comment: https://git.openjdk.org/jdk/pull/16920#discussion_r1429951237
PR Review Comment: https://git.openjdk.org/jdk/pull/16920#discussion_r1429946043
PR Review Comment: https://git.openjdk.org/jdk/pull/16920#discussion_r1429947950
