On Tue, 7 Jan 2025 09:56:11 GMT, Julian Waters <jwat...@openjdk.org> wrote:
>> After 8339120, gcc began catching many different instances of unused code in
>> the Windows specific codebase. Some of these seem to be bugs. I've taken the
>> effort to mark out all the relevant globals and locals that trigger the
>> unused warnings and addressed all of them by commenting out the code as
>> appropriate. I am confident that in many cases this simplistic approach of
>> commenting out code does not fix the underlying issue, and the warning
>> actually found a bug that should be fixed. In these instances, I will be
>> aiming to fix these bugs with help from reviewers, so I recommend anyone
>> reviewing who knows more about the code than I do to see whether there is
>> indeed a bug that needs fixing in a different way than what I did
>>
>> build.log on release configuration:
>>
>> C:/users/vertig0/downloads/eclipse-committers-2023-12-r-win32-x86_64/workspace/jdk/src/jdk.accessibility/windows/native/jabswitch/jabswitch.cpp:
>> In function 'int regEnable()':
>> C:/users/vertig0/downloads/eclipse-committers-2023-12-r-win32-x86_64/workspace/jdk/src/jdk.accessibility/windows/native/jabswitch/jabswitch.cpp:334:14:
>> warning: unused variable 'freeData' [-Wunused-variable]
>> 334 | bool freeData = false;
>> | ^~~~~~~~
>> C:/users/vertig0/downloads/eclipse-committers-2023-12-r-win32-x86_64/workspace/jdk/src/jdk.accessibility/windows/native/jabswitch/jabswitch.cpp:326:11:
>> warning: unused variable 'retval' [-Wunused-variable]
>> 326 | DWORD retval = -1;
>> | ^~~~~~
>> C:/users/vertig0/downloads/eclipse-committers-2023-12-r-win32-x86_64/workspace/jdk/src/jdk.accessibility/windows/native/jabswitch/jabswitch.cpp:
>> In function 'int regDeleteValue(HKEY, LPCWSTR)':
>> C:/users/vertig0/downloads/eclipse-committers-2023-12-r-win32-x86_64/workspace/jdk/src/jdk.accessibility/windows/native/jabswitch/jabswitch.cpp:379:14:
>> warning: unused variable 'freeData' [-Wunused-variable]
>> 379 | bool freeData = false;
>> | ^~~~~~~~
>> C:/users/vertig0/downloads/eclipse-committers-2023-12-r-win32-x86_64/workspace/jdk/src/jdk.accessibility/windows/native/jabswitch/jabswitch.cpp:367:11:
>> warning: unused variable 'retval' [-Wunused-variable]
>> 367 | DWORD retval = -1;
>> | ^~~~~~
>> C:/users/vertig0/downloads/eclipse-committers-2023-12-r-win32-x86_64/workspace/jdk/src/jdk.accessibility/windows/native/bridge/AccessBridgeCalls.c:
>> In function 'shutdownAccessBridge':
>> C:/users/vertig0/downloads/eclipse-committers-2023-12-r-win3...
>
> 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
I haven't looked at all the files, yet I'm posting my comments right away for
discussion.
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`).
src/jdk.accessibility/windows/native/jabswitch/jabswitch.cpp line 334:
> 332: TCHAR dataBuffer[DEFAULT_ALLOC];
> 333: TCHAR *data = dataBuffer;
> 334: // bool freeData = false;
This should be set to `true` if `data` is reallocated, otherwise both
`regEnable` and `regDeleteValue` leak `data`.
https://github.com/openjdk/jdk/blob/4a9fba615da0dfa6646ecb9fd9d929f74fe6875e/src/jdk.accessibility/windows/native/jabswitch/jabswitch.cpp#L336-L341
In fact, the above statement should probably be removed — the following code
uses only `dataBuffer` which does not contain any data if the buffer was
dynamically allocated and assigned to `data`.
src/jdk.accessibility/windows/native/jabswitch/jabswitch.cpp line 364:
> 362: }
> 363:
> 364: int regDeleteValue(HKEY hFamilyKey, LPCWSTR lpSubKey)
There's one more memory leak:
https://github.com/openjdk/jdk/blob/4a9fba615da0dfa6646ecb9fd9d929f74fe6875e/src/jdk.accessibility/windows/native/jabswitch/jabswitch.cpp#L352
The `delete[]` operator must be called on `newStr`.
It's present in both `regEnable` and `regDeleteValue`.
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.
src/jdk.accessibility/windows/native/jaccesswalker/jaccesswalker.cpp line 261:
> 259: GetClientRect(hWnd, &rcClient);
> 260: // hwndEdit =
> 261: CreateWindow("Edit",
The edit control is created visible and it's resized to cover the entire client
area.
The edit control is used… the text is updated in the [`displayAndLog`
function](https://github.com/openjdk/jdk/blob/c38417a86e27f047715cfd9a98770387d994a512/src/jdk.accessibility/windows/native/toolscommon/AccessInfo.cpp#L60).
Since the edit control is a child window, it's automatically destroyed when the
parent window is destroyed.
Therefore, there's no reason to store the handle in a variable, and I suggest
removing the `hwndEdit` variable.
Moreover, the comment for `hwndEdit`—`// handle of tree-view control`—is
confusing because it doesn't store a handle to a TreeView control.
src/jdk.accessibility/windows/native/jaccesswalker/jaccesswalker.cpp line 561:
> 559: tvis.item = tvi;
> 560:
> 561: /* HTREEITEM treeNodeItem = */ TreeView_InsertItem(treeWnd,
> &tvis);
Since it's unused, I'd rather remove the variable altogether.
Does the handle have any other usage except for checking whether an error
occurred?
-------------
PR Review: https://git.openjdk.org/jdk/pull/21656#pullrequestreview-2564668085
PR Review Comment: https://git.openjdk.org/jdk/pull/21656#discussion_r1924375318
PR Review Comment: https://git.openjdk.org/jdk/pull/21656#discussion_r1923750005
PR Review Comment: https://git.openjdk.org/jdk/pull/21656#discussion_r1923752261
PR Review Comment: https://git.openjdk.org/jdk/pull/21656#discussion_r1923774492
PR Review Comment: https://git.openjdk.org/jdk/pull/21656#discussion_r1923839914
PR Review Comment: https://git.openjdk.org/jdk/pull/21656#discussion_r1923849086
