On Tue, 21 Jan 2025 21:18:27 GMT, Alexey Ivanov <[email protected]> wrote:
>> Julian Waters has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains eight commits:
>>
>> - Merge branch 'openjdk:master' into accessibility
>> - Cast to void in AccessBridgeCalls.c
>> - static_cast to void in jaccessinspector.cpp
>> - Formatting changes in AccessBridgeEventHandler.cpp
>> - Merge branch 'master' into accessibility
>> - Remove now unused result
>> - Merge branch 'master' into accessibility
>> - 8342870
>
> src/jdk.accessibility/windows/native/bridge/AccessBridgeCalls.c line 230:
>
>> 228: if (result != TRUE) {
>> 229: error = GetLastError();
>> 230: }
>
> The comparison above should be `if (result == 0)` instead of `!= TRUE`,
> because
> [`FreeLibrary`](https://learn.microsoft.com/en-us/windows/win32/api/libloaderapi/nf-libloaderapi-freelibrary)
> returns non-zero if the function succeeds, which implies the return value
> could be different from 1 (`TRUE`).
Fixed. I also changed the logic below to return FALSE if the FreeLibrary call
above failed
> src/jdk.accessibility/windows/native/jaccessinspector/jaccessinspector.cpp
> line 1212:
>
>> 1210: }
>> 1211:
>> 1212: static_cast<void>(lastError);
>
> This falls into category that's useful for debugging only. The value of
> `lastError` is never used to report an error, yet one can use a debugger to
> inspect the value.
>
> There seems to be no way to report or log the error.
>
> Isn't it better to add a flag to suppress the warning? Casting `lastError` to
> `void` is cryptic. A comment would definitely be helpful here.
I suppose I could do that yes, if that that is preferred
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/21656#discussion_r1926387441
PR Review Comment: https://git.openjdk.org/jdk/pull/21656#discussion_r1926388732
