[code-reflection] RFR: Exception regions fix

[Paul Sandoz]

On Wed, 31 Jan 2024 15:55:41 GMT, Adam Sotona <asotona at openjdk.org> wrote:

> BytecodeGenerator::computeExceptionRegionMembership now collects all necessary info into BitSets and directly declares the regions with CodeBuilder.
> All tests are now passing.
> 
> Please review.
> 
> Thanks,
> Adam

Very nicely done, just a few minor comments.

This is much better than my original approach, and nicely separated from the main loop translating operations to bytecode instructions.

src/java.base/share/classes/java/lang/reflect/code/bytecode/BytecodeGenerator.java line 347:

> 345: 
> 346: 
> 347:     private static void computeExceptionRegionMembership(Body r, CodeBuilder cob, ConversionContext c) {

Let's rename `r` to `body` (then it does not cause confusion with `r` used for the active region index.

src/java.base/share/classes/java/lang/reflect/code/bytecode/BytecodeGenerator.java line 355:

> 353:         class BlockWithActiveExceptionRegions {
> 354:             final Block block;
> 355:             final BitSet activeRegions;

Clever use of a bit set to represent the active stack of regions. Perhaps rename to `activeRegionStack`?

src/java.base/share/classes/java/lang/reflect/code/bytecode/BytecodeGenerator.java line 359:

> 357:                 this.block = block;
> 358:                 this.activeRegions = activeRegions;
> 359:                 int index = blocks.indexOf(block);

We should be able to use `block.index()`, however that is not fully working as intended right now and needs to be fixed (i.e., this invariant should hold `block.index() == blocks.indexOf(block)`, but currently does not)

src/java.base/share/classes/java/lang/reflect/code/bytecode/BytecodeGenerator.java line 381:

> 379:             } else if (top instanceof CoreOps.ExceptionRegionEnter er) {
> 380:                 ArrayList<Block.Reference> catchBlocks = new ArrayList<>(er.catchBlocks());
> 381:                 for (Block.Reference catchBlock : catchBlocks.reversed()) {

No need for additional array list:
Suggestion:

                for (Block.Reference catchBlock : er.catchBlocks().reversed()) {

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

PR Review: https://git.openjdk.org/babylon/pull/12#pullrequestreview-1854711028
PR Review Comment: https://git.openjdk.org/babylon/pull/12#discussion_r1473392037
PR Review Comment: https://git.openjdk.org/babylon/pull/12#discussion_r1473395910
PR Review Comment: https://git.openjdk.org/babylon/pull/12#discussion_r1473393579
PR Review Comment: https://git.openjdk.org/babylon/pull/12#discussion_r1473394388