On Thu, 19 Jun 2025 06:24:03 GMT, David Holmes <dhol...@openjdk.org> wrote:
>> Coleen Phillimore has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Update src/hotspot/share/prims/jvmtiRedefineClasses.cpp
>>
>> Co-authored-by: David Holmes
>> <62092539+dholmes-...@users.noreply.github.com>
>
> src/hotspot/share/classfile/stackMapTable.hpp line 154:
>
>> 152: SAME_FRAME = 64,
>> 153: SAME_LOCALS_1_STACK_ITEM_FRAME = 128,
>> 154: SAME_LOCALS_1_STACK_ITEM_EXTENDED = 247,
>
> I find these definitions a little confusing. SAME_FRAME is actually 0-63,
> with SAME_LOCALS_1_STACK_ITEM_FRAME being 64-127. Given many of these frame
> types imply tag ranges it may be clearer to define enum's for the start and
> end of ranges as applicable eg.
>
> enum {
> SAME_FRAME_START = 0,
> SAME_FRAME_END = 63,
> SAME_LOCALS_1_STACK_ITEM_FRAME_START = 64,
> SAME_LOCALS_1_STACK_ITEM_FRAME_END = 127,
> RESERVED_START = 128,
> RESERVED_END = 246,
> SAME_LOCALS_1_STACK_ITEM_EXTENDED = 247,
> CHOP_FRAME_START = 248,
> CHOP_FRAME_END = 250,
> SAME_FRAME_EXTENDED = 251,
> APPEND_FRAME_START = 252,
> APPEND_FRAME_END = 254,
> FULL_FRAME = 255
> }
>
> and then adjust the code usage as appropriate e.g.
>
> if (frame_type <= SAME_FRAME_END) {
> ...
> if (frame_type <= SAME_LOCALS_1_STACK_ITEM_FRAME_END) {
> if (_first) {
> offset = frame_type - SAME_LOCALS_1_STACK_ITEM_FRAME_START;
> ...
>
> What do you think?
I wasn't really up for a big rewrite but having the complete set of names would
be really good.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/25870#discussion_r2158789990
