Revisit j.l.classfile.CodeBuilder API surface

[Brian Goetz]

One problem with `newLocalSlot` is that it now conflicts with the new 
instructions like `newObject`, and thus makes it less clear whether the 
method is _generating_ bytecode or merely setting up for a future 
operation.  But perhaps `allocateSlot` or `allocateLocalSlot` or 
`reserveLocalSlot` is more clear.

On 1/5/2024 11:21 AM, - wrote:
> 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.
>>
>>     Majorityof the methods may be dividedinto 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
>>     <https://urldefense.com/v3/__https://github.com/openjdk/jdk/pull/17282__;!!ACWV5N9M2RV99hQ!K4zh9o6NcHVlQHameNjhwvRGo_HnBCYhzTH9PQDru-SZu5QkkI1NPfSZHyiCN_7l2PlyEU41N_Fp-LCMfbzTFjDa$>
>>
>>     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/533b72ef/attachment-0001.htm>