blackdrag commented on code in PR #2706:
URL: https://github.com/apache/groovy/pull/2706#discussion_r3572755044
##########
src/test/groovy/bugs/Groovy11362.groovy:
##########
@@ -33,9 +33,11 @@ final class Groovy11362 extends AbstractBytecodeTestCase {
}
}
'''
+ // Catch parameter must be typed Exception (GROOVY-11362), not Object.
+ // Label numbers are not stable across try/catch codegen improvements.
Review Comment:
They had been stable before, why not anymore?
##########
src/main/java/org/codehaus/groovy/classgen/asm/CompileStack.java:
##########
@@ -941,19 +951,19 @@ private void applyBlockRecorder(final
Collection<BlockRecorder> blockRecorders)
if (visitedBlocks.contains(recorder)) continue;
Label end = new Label();
+ // Guarantee a non-empty protected range before excluding finally.
mv.visitInsn(NOP);
mv.visitLabel(end);
-
recorder.closeRange(end);
- // we exclude the finally block from the exception table
- // here to avoid double visiting of finally statements
+ // Exclude the finally body from the exception table so it is not
+ // re-entered if it throws (avoid double application of finally).
recorder.excludedStatement.run();
recorder.startRange(start);
}
- mv.visitInsn(NOP);
+ // start marks the first instruction after all inlined finally blocks.
mv.visitLabel(start);
Review Comment:
I vaguely remember I did need those NOP instructions or else I could not set
the label. I think this was for cases in which this was the first instruction
in the method... maybe for an empty try-finally? not sure anymore. Or maybe I
added it here back then defensively because of a similar problem in
combinations with returns? But thinking back I don't think this was because of
the bytecode, I think it was because of ASM. So either a bug or me not using it
correctly. Very possible this is no longer an issue and thus not required.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
