Skip to content

Final ASMAPI cleanup, NumberType enum change, and new insertInsnList method #54

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
LexManos merged 4 commits into from
Oct 16, 2024
Merged

Final ASMAPI cleanup, NumberType enum change, and new insertInsnList method #54

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cbf40b3
Renamed appendMethodCall to injectMethodCall
Jonathing Oct 15, 2024
1695e6c
Rename NumberType, widen usage with castNumber
Jonathing Oct 15, 2024
7eb8458
Add insertInsnList with a single insn reference
Jonathing Oct 15, 2024
bc629a9
Make the old insertInsnList use the new one
Jonathing Oct 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
101 changes: 67 additions & 34 deletions src/main/java/net/minecraftforge/coremod/api/ASMAPI.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
import java.io.PrintWriter;
import java.io.StringWriter;
import java.lang.reflect.Modifier;
import java.util.Iterator;
import java.util.ListIterator;
import java.util.Objects;
import java.util.Optional;
Expand All @@ -33,17 +32,29 @@ public static MethodNode getMethodNode() {
return new MethodNode(Opcodes.ASM9);
}

// Terribly named method. Should be called "prependMethodCall" or something similar.
/**
* Injects a method call to the beginning of the given method.
*
* @param node The method to inject the call into
* @param methodCall The method call to inject
*/
public static void appendMethodCall(MethodNode node, MethodInsnNode methodCall) {
public static void injectMethodCall(MethodNode node, MethodInsnNode methodCall) {
node.instructions.insertBefore(node.instructions.getFirst(), methodCall);
}

/**
* Injects a method call to the beginning of the given method.
*
* @param node The method to inject the call into
* @param methodCall The method call to inject
*
* @deprecated Renamed to {@link #injectMethodCall(MethodNode, MethodInsnNode)}
*/
@Deprecated(forRemoval = true, since = "5.1")
Copy link
Member Author

@Jonathing Jonathing Oct 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

public static void appendMethodCall(MethodNode node, MethodInsnNode methodCall) {
injectMethodCall(node, methodCall);
}

/**
* Signifies the method invocation type. Mirrors "INVOKE-" opcodes from ASM.
*/
Expand Down Expand Up @@ -80,17 +91,17 @@ public static MethodInsnNode buildMethodCall(final String ownerName, final Strin
}

/**
* Signifies the type of number constant for a {@link LdcNumberType}.
* Signifies the type of number constant for a {@link NumberType}.
*/
public enum LdcNumberType {
public enum NumberType {
Copy link
Member

@LexManos LexManos Oct 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was this renamed/does it break compatibility? How old is this?

Copy link
Member Author

@Jonathing Jonathing Oct 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@LexManos LexManos Oct 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, then if you're done i can pull this

Copy link
Member Author

@Jonathing Jonathing Oct 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah it's ready

INTEGER(Number::intValue),
FLOAT(Number::floatValue),
LONG(Number::longValue),
DOUBLE(Number::doubleValue);

private final Function<Number, Object> mapper;

LdcNumberType(Function<Number, Object> mapper) {
NumberType(Function<Number, Object> mapper) {
this.mapper = mapper;
}

Expand All @@ -100,14 +111,29 @@ public Object map(Number number) {
}

/**
* Builds a new {@link LdcInsnNode} with the given number value and type.
* Casts a given number to a given specific {@link NumberType}. This helps elliviate the problems that comes with JavaScript's
* ambiguous number system.
* <p>
* The result is returned as an {@link Object} so it can be used as a value in various instructions that require
* values.
*
* @param value The number to cast
* @param type The type of number to cast to
* @return The casted number
*/
public static Object castNumber(final Number value, final NumberType type) {
return type.map(value);
}

/**
* Builds a new {@link LdcInsnNode} with the given number value and {@link NumberType}.
*
* @param value The number value
* @param type The type of the number
* @return The built LDC node
*/
public static LdcInsnNode buildNumberLdcInsnNode(final Number value, final LdcNumberType type) {
return new LdcInsnNode(type.map(value));
public static LdcInsnNode buildNumberLdcInsnNode(final Number value, final NumberType type) {
return new LdcInsnNode(castNumber(value, type));
}

/**
Expand Down Expand Up @@ -361,35 +387,42 @@ public static MethodInsnNode findFirstMethodCallBefore(MethodNode method, Method
* in the method provided. Only the first node matching is targeted, all other matches are ignored.
*
* @param method The method where you want to find the node
* @param type The type of the old method node.
* @param owner The owner of the old method node.
* @param name The name of the old method node. You may want to use {@link #mapMethod(String)} if this is a srg
* name
* @param desc The desc of the old method node.
* @param type The type of the old method node
* @param owner The owner of the old method node
* @param name The name of the old method node (you may want to use {@link #mapMethod(String)} if this is a srg
* name)
* @param desc The desc of the old method node
* @param list The list that should be inserted
* @param mode How the given code should be inserted
* @return True if the node was found, false otherwise
* @return True if the node was found and the list was inserted, false otherwise
*/
public static boolean insertInsnList(MethodNode method, MethodType type, String owner, String name, String desc, InsnList list, InsertMode mode) {
Iterator<AbstractInsnNode> nodeIterator = method.instructions.iterator();
int opcode = type.toOpcode();
while (nodeIterator.hasNext()) {
AbstractInsnNode next = nodeIterator.next();
if (next.getOpcode() == opcode) {
MethodInsnNode castedNode = (MethodInsnNode) next;
if (castedNode.owner.equals(owner) && castedNode.name.equals(name) && castedNode.desc.equals(desc)) {
if (mode == InsertMode.INSERT_BEFORE)
method.instructions.insertBefore(next, list);
else
method.instructions.insert(next, list);

if (mode == InsertMode.REMOVE_ORIGINAL)
nodeIterator.remove();
return true;
}
}
}
return false;
var insn = findFirstMethodCall(method, type, owner, name, desc);
if (insn == null) return false;

return insertInsnList(method, insn, list, mode);
}

/**
* Inserts/replaces a list after/before the given instruction.
*
* @param method The method where you want to insert the list
* @param list The list that should be inserted
* @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) {
Copy link
Member Author

@Jonathing Jonathing Oct 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

@LexManos LexManos Oct 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having it return a boolena means the CoreMod itself can determine to crash or not

if (!method.instructions.contains(insn)) return false;

if (mode == InsertMode.INSERT_BEFORE)
method.instructions.insertBefore(insn, list);
else
method.instructions.insert(insn, list);

if (mode == InsertMode.REMOVE_ORIGINAL)
method.instructions.remove(insn);

return true;
}

/**
Expand Down