fix warning about Field.isAccessible() #1927
Conversation
Test Results 1 206 files - 606 1 206 suites - 606 1h 5m 36s ⏱️ - 31m 51s For more details on these failures, see this check. Results for commit 49cf3a9. ± Comparison against base commit 1af7f4d. This pull request skips 3 tests.♻️ This comment has been updated with latest results. |
| contextId = (String) method.invoke(command); | ||
| if (method.trySetAccessible()) { | ||
| contextId = (String) method.invoke(command); | ||
| } | ||
| } catch (Exception e) { | ||
| // do nothing | ||
| } |
There was a problem hiding this comment.
wouldn't the finally block also need be adjusted?
There was a problem hiding this comment.
Also: Command#getHelpContextId() says clients shally use CommandManager#getHelpContextId(Command) instead. Can't we call that here?
There was a problem hiding this comment.
there is no final block here, `CommandManager does a lot more then just calling the method. I don't want to break anything.
There was a problem hiding this comment.
ah, right, there is no finally block here. but the same line of thought applies:
The condition is changed from isAccessible() to canAccess(), and now the defensive if(tryAccess()) is used, but in line 266 setAccessible() is still called unconditionally which can then still fail.
I propose to also move that 'undo/cleanup' line inside the if(try) block, since otherwise there is nothing to undo and it could needlessly raise an exception.
| value = field.get(instance); | ||
| if (field.trySetAccessible()) { | ||
| value = field.get(instance); | ||
| } | ||
| } catch (Exception exc) { | ||
| // do nothing | ||
| } finally { |
There was a problem hiding this comment.
finally block is not symmetric anymore. I think the finally can be moved inside the if(try) block.
There was a problem hiding this comment.
i don't get what you mean with "symmetric" here or how moving the try block should help, but OK, won't hurt either.
jukzi
force-pushed
the
isAccessible
branch
4 times, most recently
from
49dcd44
to
43d14dc
Compare
'The method isAccessible() from the type AccessibleObject is deprecated since version 9' tested with WorkbenchThemeChangedHandlerTest, org.eclipse.ui.tests.commands.HelpContextIdTest.testHelpContextId()
|
unrelated random fails: |
'The method isAccessible() from the type AccessibleObject is deprecated since version 9'
tested with
WorkbenchThemeChangedHandlerTest,
org.eclipse.ui.tests.commands.HelpContextIdTest.testHelpContextId()