On Tue, 9 Dec 2025 20:59:58 GMT, Trevor Bond <[email protected]> wrote:
> Add a new factory method to `IncrementInstruction` that allows the explicit
> creation of a wide iinc instruction, even with a `slot` and `constant` that
> could fit into a normal iinc instruction. Previously, only one factory for
> iinc instructions existed, which inferred the type of instruction needed
> given the size of `slot` and `constant`. Add additional test cases for the
> new factory as well. All tier 1 tests and classfile tests have passed with
> these changes.
Since this is an API addition, this will require a CSR. Once we have settled
with the javadocs, I can create a CSR (which requires a JBS account)
src/java.base/share/classes/java/lang/classfile/instruction/IncrementInstruction.java
line 95:
> 93: * <p>
> 94: * The ranges of {@code slot} and {@code constant} are restricted by
> the
> 95: * {@code op} and its {@linkplain Opcode#sizeIfFixed() size}:
Since there are only two possible Opcodes `IINC` and `IINC_W`, you can just
name them instead of specifying by their size.
I know you copied from the load and store instructions; I had to use size there
because listing all of `ILOAD_0`, `ILOAD_1`, `ILOAD_2`, `ILOAD_3` is less
concise than using the size.
You can look at `DiscontinuedInstruction.RetInstruction` for a better template
to follow.
src/java.base/share/classes/java/lang/classfile/instruction/IncrementInstruction.java
line 118:
> 116: * {@link Opcode.Kind#INCREMENT} or {@code slot} or
> 117: * {@code constant} is out of range
> 118: */
Suggestion:
* {@code constant} is out of range
* @since 27
*/
src/java.base/share/classes/jdk/internal/classfile/impl/BytecodeHelpers.java
line 456:
> 454: boolean requiresWide = validateAndIsWideIinc(slot, constant);
> 455: int size = opcode.sizeIfFixed();
> 456: if ((size == 3 && requiresWide)) {
Suggestion:
if (validateAndIsWideIinc(slot, constant) && opcode != Opcode.IINC_W) {
test/jdk/jdk/classfile/InstructionValidationTest.java line 265:
> 263:
> 264: @Test
> 265: void testExplicitIincConstant() {
Each method tests one instruction interface, so I think you should merge this
into `testIincConstant()`.
-------------
PR Comment: https://git.openjdk.org/jdk/pull/28729#issuecomment-3634775601
PR Review Comment: https://git.openjdk.org/jdk/pull/28729#discussion_r2604710736
PR Review Comment: https://git.openjdk.org/jdk/pull/28729#discussion_r2604730390
PR Review Comment: https://git.openjdk.org/jdk/pull/28729#discussion_r2604718223
PR Review Comment: https://git.openjdk.org/jdk/pull/28729#discussion_r2604727924
