jdk
jdk copied to clipboard
openjdk
8342870: Errors related to unused code on Windows after 8339120 in accessibility
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
Progress
- [ ] Change must be properly reviewed (1 review required, with at least 1 Reviewer)
- [x] Change must not contain extraneous whitespace
- [x] Commit message must refer to an issue
Issue
- JDK-8342870: Errors related to unused code on Windows after 8339120 in accessibility (Bug - P4)
Reviewing
Using git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/21656/head:pull/21656
$ git checkout pull/21656
Update a local copy of the PR:
$ git checkout pull/21656
$ git pull https://git.openjdk.org/jdk.git pull/21656/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 21656
View PR using the GUI difftool:
$ git pr show -t 21656
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/21656.diff
Webrev
:wave: Welcome back jwaters! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.
![bridgekeeper[bot] avatar](https://avatars.githubusercontent.com/u/41768318?v=4 "Published by a pub.dev verified publisher"){kind=small}
❗ This change is not yet ready to be integrated. See the Progress checklist in the description for automated requirements.
@TheShermanTanker The following label will be automatically applied to this pull request:
client
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.
Webrevs
- 06: Full - Incremental (690daaf5)
- 05: Full - Incremental (47b168a3)
- 04: Full (f975dfa2)
- 03: Full - Incremental (b91d2f3c)
- 02: Full (324f89f1)
- 01: Full - Incremental (de6afdb1)
- 00: Full (821307b5)
There's not much I like about this PR. I think it would be better to withdraw it and submit a bug against A11Y for people to look at what should really be done about these cases. Which could include "do nothing".
There's not much I like about this PR. I think it would be better to withdraw it and submit a bug against A11Y for people to look at what should really be done about these cases. Which could include "do nothing".
Thanks for taking a look. I'm not sure who is responsible for reviewing A11Y though, so I don't know who to submit the bug to for them to take a look at
Also, as mentioned, the fixes in here might not necessarily be correct. These are only stopgap changes to draw attention to the areas causing an issue, so they can be properly caught and fixed if need be
@prrace Sorry for the trouble, but are there other maintainers for accessibility that can take a look at this change? I know you can't really review this properly since you're pretty busy, so I was thinking of trying to get the attention of other maintainers, but I unfortunately don't know who they are
@prrace I've tried to fix up the code changes here to look as neat as possible, but ultimately I'm not sure what you want it to look like. Could you explain why you don't like the changes in accessibility? Unlike the corresponding awt changes which are more complex and even touch the C++ Standard, the Pull Request here consists purely of marking unused code for removal, which shouldn't be too bad at least in my opinion, which is why I'm not sure why you don't like it
Sorry for this, but I really have no one else to turn to for help. Paging @azuev-java
@TheShermanTanker This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!
@TheShermanTanker This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.
@TheShermanTanker This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!
@TheShermanTanker This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply issue a /touch or /keepalive command to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!
@TheShermanTanker The pull request is being re-evaluated and the inactivity timeout has been reset.
@TheShermanTanker This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply issue a /touch or /keepalive command to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!
@TheShermanTanker The pull request is being re-evaluated and the inactivity timeout has been reset.