Skip to content
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.

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

Add InsnType for precise instruction searching, additional cleanup #49

Merged

Conversation

Jonathing
Copy link
Member

@Jonathing Jonathing commented Oct 11, 2024

Depends on #48.

This PR's main feature is to addition of enum InsnType, which allows for more precise instruction searching within methods. This reduces the need of the end user to continuously make for loops to traverse through method instructions.

Additionally, this PR cleans up formatting, updates getMethodNode() to use Opcodes.ASM9, and adds a new method insnToString() to allow debugging of single instructions.

@Jonathing Jonathing marked this pull request as draft October 11, 2024 01:54
@Jonathing Jonathing changed the title A Add InsnType for precise instruction searching, additional cleanup Oct 11, 2024
@Jonathing
Copy link
Member Author

Pro-tip: hitting ENTER by accident while writing the title of a PR instantly creates it without confirmation!

* @see AbstractInsnNode
*/
public enum InsnType {
INSN,
Copy link
Member

Choose a reason for hiding this comment

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

This has the ordinal compared to the ASM type number. Is there a way we can just expose the ASM type as API instead so we don't have to keep this enum in sync with ASM?

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, so these types are merely mimicking the constants in AbstractInsnNode. This enum was my simplest implementation of it. It is possible to directly invoke them by importing that class in the JS file, but that's what I'm trying to avoid...

Copy link
Member

Choose a reason for hiding this comment

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

Humm, it would make the code more verbose, but less error prone to instead just copy these constants here instead of using a enum and hoping the ordinals align.
Not sure what the best option would be here. This works fine. But I just dont like the fragility of it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I am not opposed to verbosity, but I don't want to bloat it either. Given that it would only benefit us to directly link them, I might as well just do that.

Copy link
Member

Choose a reason for hiding this comment

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

This is a much better/easier to maintain approach, with minimal code uglyness, i like it.

@Jonathing
Copy link
Member Author

Marking this PR as ready for review. All the code is very straight forward, shouldn't cause any issues, and is overall rather concise. Unfortunately with tests in this project currently being broken, it is very hard to write tests within the project itself without needing to publish it to maven local and running it in Forge itself.

@Jonathing Jonathing marked this pull request as ready for review October 14, 2024 00:21
@LexManos LexManos merged commit c3dac8c into MinecraftForge:master Oct 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants