Do not report DLS_DEAD_LOCAL_STORE for java 21 type switch variables
#2828
Conversation
| import org.apache.bcel.generic.Instruction; | ||
| import org.apache.bcel.generic.InstructionHandle; | ||
| import org.apache.bcel.generic.ASTORE; | ||
| import org.apache.bcel.generic.Instruction; |
There was a problem hiding this comment.
Line 31 and 34 are the same imports.
| switchHandler.enterSwitch(switchInstruction.getOpcode(), | ||
| pc, | ||
| indices, | ||
| 0, // Not sure how to get the default offset from BCEL but it doesn't matter here | ||
| false); // It shouldn't matter here if the switch was exhaustive or not |
There was a problem hiding this comment.
Can you please take a look at the definition of this method? The last parameter, which is a primitive boolean, has the annotation @CheckForNull.
It was added in #2813, which was already merged, but you are the author. Are you sure, that this was what you wanted?
There was a problem hiding this comment.
I have removed the @CheckForNull, I mistakenly carried it over when overloading the method
| InstructionHandle handle = location.getHandle(); | ||
| int pc = handle.getPosition(); |
There was a problem hiding this comment.
It's great that you moved these into separate variables. Can you please use these inside this method instead of retrieving it by calling the functions several times?
There was a problem hiding this comment.
I actually moved these up because the variables were declared further down and I needed them for the new code.
If that's OK I'd like to keep the existing code untouched, so the changes are limited to fixing this false positive.
| @@ -254,18 +262,41 @@ private void analyzeMethod(ClassContext classContext, Method method) throws Data | |||
| BugInstance pendingBugReportAboutOverwrittenParameter = null; | |||
| try { | |||
| WarningPropertySet<WarningProperty> propertySet = new WarningPropertySet<>(); | |||
|
|
|||
There was a problem hiding this comment.
This method is quite long as it is, with this change, ranging from line 207 to 645. IMO it's worth considering moving some parts of it into separate functions. (Not necessarily in this PR.)
| InstructionHandle handle = location.getHandle(); | ||
| int pc = handle.getPosition(); | ||
|
|
||
| if (handle.getInstruction() instanceof INVOKEDYNAMIC) { |
There was a problem hiding this comment.
Maybe handle.getInstruction() also could be moved into a variable.
|
Thanks for reviewing @JuditKnoll, I have made some changes |
As noted in #2736 there's a mandatory variable in java 21 type switches, SpotBugs should not report
DLS_DEAD_LOCAL_STOREwhen that variable remains unused.Build on top of #2782 to detect that the problematic
astorewas part of a type switch and ignore it.