RFR: 8342870: Errors related to unused code on Windows after 8339120 in accessibility [v5]

[Alexander Zuev]

On Tue, 7 Jan 2025 09:56:11 GMT, Julian Waters <jwaters at 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

src/jdk.accessibility/windows/native/jaccesswalker/jaccesswalker.cpp line 255:

> 253:     case WM_CREATE:
> 254:         RECT rcClient;    // dimensions of client area
> 255:      // HWND hwndEdit;    // handle of tree-view control

Again, i would not remove or comment out variables assigned within the code - they might not be used down the line but in interactive debug session it is good to have them to see what was the result of the function call without stepping inside the function. I agree that removing the declarations of unused variables can be justified but this in my opinion is not the case.

src/jdk.accessibility/windows/native/libwindowsaccessbridge/AccessBridgeJavaVMInstance.cpp line 271:

> 269:             DEBUG_CODE(if (*type == cGetAccessibleTextItemsPackage) {)
> 270:                 DEBUG_CODE(AppendToCallInfo("  'memoryMappedView' now contains:"));
> 271:              // DEBUG_CODE(GetAccessibleTextItemsPackage *pkg = (GetAccessibleTextItemsPackage *) (buffer + sizeof(PackageType)));

Why do we need to comment out that? In non-debug build it will be replaced by the comment block anyways.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/21656#discussion_r1922911678
PR Review Comment: https://git.openjdk.org/jdk/pull/21656#discussion_r1922911741