Revisit j.l.classfile.CodeBuilder API surface

[Adam Sotona]

Hi,
I’ve updated the PR<https://github.com/openjdk/jdk/pull/17306> draft with removal of operator method as obsolete and
newPrimitiveArray, newReferenceArray and newMultidimensionalArray methods as duplicate.

I’ve also extended conversion method to support the whole matrix (including multi-step conversions, no-op conversions and BooleanType conversions), see CodeBuilder.java#L548<https://github.com/openjdk/jdk/blob/027374004c9c9bf53a709c98593363043e8b7782/src/java.base/share/classes/java/lang/classfile/CodeBuilder.java#L548>

Regarding the allocateLocal I would slightly prefer to keep it, however allocateSlot or allocateLocalSlot or reserveLocalSlot are also fine.
Prefix new would be confusing.

Thanks,
Adam



From: Brian Goetz <brian.goetz at oracle.com>
Date: Friday, 5 January 2024 at 17:33
To: liangchenblue at gmail.com <liangchenblue at gmail.com>, classfile-api-dev <classfile-api-dev at openjdk.org>
Cc: Adam Sotona <adam.sotona at oracle.com>
Subject: Re: Revisit j.l.classfile.CodeBuilder API surface
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<mailto: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?

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/classfile-api-dev/attachments/20240109/3478dd84/attachment-0001.htm>