Revisit j.l.classfile.CodeBuilder API surface

[-]

I love this simplification too!

One nitpick is that now we have `loadLocal` and `storeLocal`, maybe we
should rename existing `allocateLocal` to something like `newLocalSlot`
("allocate" is disliked by panama devs too), so it's clear it returns a
slot index like `parameterSlot` or `receiverSlot` do.

In addition, the previous "instruction family" APIs have their downfalls
too: some families are not complete, and may throw exceptions upon
combination of certain arguments but are not well documented. For example,
a `conversion(TypeKind.LongType, TypeKind.ByteType)` would fail, because
bytecode actually uses 2 instructions: l2i and i2b. (maybe upgrading this
`conversion` to emit multiple bytecode is a solution, too) Same for calling
`newPrimitiveArray(TypeKind.VoidType)` etc. Similarly, the
`operator(Opcode)` is just a watered-down version of ASM's `visitInsn`, yet
is more risky as you can pass in other no-operand opcodes that fail at
runtime without compile-time hints.

In contrast, the more specific methods in CodeBuilder avoids these pitfalls.

After all, my rant above is not against this change, but instead are
improvements that are enabled by this simplification.

Cheers,
Chen Liang

On Fri, Jan 5, 2024 at 9:52 AM Brian Goetz <brian.goetz at oracle.com> wrote:

> Let me fill in some history here.
>
> Much of the design process for this library could be described as an
> iterated game of "find the primitive."  The first version of the API had a
> zillion ad-hoc methods, with little structural relation.  At some point we
> hit upon a model-driven analysis, and broke down instructions into families
> (loads, stores, etc), and at that point, we thought methods like
> loadInstruction were the primitives.  But we hadn't actually hit bottom
> yet; after many rounds of refactoring, it became clear that there was one
> primitive for the builder hierarchy, now called `with`, and all of the
> builder methods now bottom out at that.
>
> At the same time, we added a number of "convenience" methods such as
> aload_0() and aload(int n) (which now just delegate to with() or
> loadInstruction()), and over time, we found that most generative use cases
> used these methods much more than the primitives (which is fine.)
>
> At this point, I think many of the xxxInstruction (and its rigid naming
> convention) are vestiges of a previous attempt to organize the API.  Some
> still have uses, though should be renamed to reflect that they are mere
> conveniences (such as arrayLoadInstruction to arrayLoad).
>
> (Does anyone use operator(opcode), or can we drop that one too?)
>
> So I support this simplification.
>
> Having done the refactor, were there any surprises in the usages of
> various CodeBuilder methods?
>
>
> On 1/5/2024 10:38 AM, Adam Sotona wrote:
>
> Hi,
>
> j.l.classfile.CodeBuilder API consist of more than 230 methods.
>
> Existing ClassFile API use cases proved the concept of one big
> CodeBuilder is comfortable. However there are some redundancies, glitches
> in the naming, some frequently used methods are hard to find and some
> methods have low practical use.
>
>
>
> Majority of the methods may be divided into three main levels:
> 1. methods building low level bytecode instructions according to JVMS
> chapter 6.5 (aaload, aastore, aconst_null...)
> 2. methods reconstructing individual subtypes of
> j.l.classfile.Instruction from given arguments (loadInstruction,
> storeInstruction, incrementInstruction, branchInstruction...)
> 3. methods modeling high level code blocks (block, ifThen, ifThenElse,
> trying...)
>
>
>
> Many methods from level 2 (with suffix `Instruction`) seem to be obsolete
> or misplaced. Some of them are duplicates of methods from level 1, some are
> obsolete and some are very useful, however a bit hidden. The API should
> be cleaned a bit while in preview.
>
> I would like to open a discussion on the following proposed changes in the
> CodeBuilder methods:
>
>    - incrementInstruction remove as duplicate of iinc
>    - lookupSwitchInstruction remove as duplicate of lookupswitch
>    - tableSwitchInstruction remove as duplicate of tableswitch
>    - throwInstruction remove as duplicate of athrow
>    - invokeDynamicInstruction remove as duplicate of invokedynamic
>    - stackInstruction remove as obsolete with suggested replacements:
>    with(StackInstruction.of(...))
>    - monitorInstruction remove as obsolete with suggested replacements:
>    monitorenter, monitorexit, or with(MonitorInstruction.of(...))
>    - nopInstruction remove as duplicate of nop
>    - typecheckInstruction remove as obsolete with suggested replacements:
>    checkcast, instanceOf, or with(TypeCheckInstruction.of(...))
>    - loadInstruction rename to loadLocal
>    - storeInstruction rename to storeLocal
>    - branchInstruction rename to branch
>    - invokeInstruction rename to invoke
>    - newObjectInstruction rename to newObject
>    - newPrimitiveArrayInstruction rename to newPrimitiveArray
>    - newReferenceArrayInstruction rename to newReferenceArray
>    - newMultidimensionalArrayInstruction rename to
>    newMultidimensionalArray
>    - arrayLoadInstruction rename to arrayLoad
>    - arrayStoreInstruction rename to arrayStore
>    - convertInstruction rename to conversion
>    - operatorInstruction rename to operator
>    - constantInstruction rename to loadConstant
>    - fieldInstruction rename to fieldAccess
>    - instanceof_ rename to instanceOf
>    - returnInstruction rename to return_
>
> Here is the related RFE: https://bugs.openjdk.org/browse/JDK-8323058
>
> Draft of the CSR (no spec yet):
> https://bugs.openjdk.org/browse/JDK-8323067
>
> And draft of the Pull Request: https://github.com/openjdk/jdk/pull/17282
>
>
>
> Any comments are welcome.
>
>
>
> Thank you,
>
> Adam
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/classfile-api-dev/attachments/20240105/1ad126f7/attachment-0001.htm>