Final ASMAPI cleanup, NumberType enum change, and new insertInsnList method #54
Conversation
| * | ||
| * @deprecated Renamed to {@link #injectMethodCall(MethodNode, MethodInsnNode)} | ||
| */ | ||
| @Deprecated(forRemoval = true, since = "5.1") |
There was a problem hiding this comment.
I've marked this method as deprecated for removal, but should it be removed? It's existed in CoreMods for a long time and a lot of older CoreMods, some of which might still be used today in FMLOnly-type mods, might be using this method.
| * @param mode How the given code should be inserted | ||
| * @return True if the list was inserted, false otherwise | ||
| */ | ||
| public static boolean insertInsnList(MethodNode method, AbstractInsnNode insn, InsnList list, InsertMode mode) { |
There was a problem hiding this comment.
This method returns boolean to have parity with the older insertInsnList method. However, I think that it might be better to crash the game if a modder tries to insert a list into the instructions using an instruction that is not included in the method. What do you think?
There was a problem hiding this comment.
Having it return a boolena means the CoreMod itself can determine to crash or not
| */ | ||
| public enum LdcNumberType { | ||
| public enum NumberType { |
There was a problem hiding this comment.
Why was this renamed/does it break compatibility? How old is this?
There was a problem hiding this comment.
It was renamed because it is no longer exclusive to LdcInsnNode. There aren't any versions of Forge that have bumped CoreMods recently to include LdcNumberType, so it doesn't break compatibility.
There was a problem hiding this comment.
Okay, then if you're done i can pull this
This PR is, what I hope to be, my final cleanup PR for ASMAPI. There were a lot of things with it I wanted to change so that it could see better usage in CoreMods without needing to resort to (too much) extra work on the modder's part, given that CoreMods is sandboxed.
appendMethodCalltoinjectMethodCall.LdcNumberTypetoNumberType.castNumber, a method which returns an Object of the desired value casted to the given NumberType.FieldNode. This approach allows modders to cast numbers themselves without needing to rely on us to make additional stray methods into ASMAPI.insertInsnListmethod which acceptsAbstractInsnNodeas opposed to the components of aMethodInsnNode.InsertMode, reducing the need to interface with the method node's instruction list directly.insertInsnListnow uses this new method along withfindFirstMethodCall, which is essentially what it was doing in the first place.My goal with these ASMAPI changes is to have them be added into Forge builds as far back as 1.18.2. These old versions only have so much to gain from these changes. The only logical breaking change is the bug fix to
findFirstInstructionBeforewhich was added in #50.Once this is merged, we can look into enhancing the Nashorn engine that CoreMods uses as described in this wishlist.