-
-
Notifications
You must be signed in to change notification settings - Fork 23
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
Add InsnType for precise instruction searching, additional cleanup #49
Conversation
Pro-tip: hitting ENTER by accident while writing the title of a PR instantly creates it without confirmation! |
729844b
to
c1db0a8
Compare
* @see AbstractInsnNode | ||
*/ | ||
public enum InsnType { | ||
INSN, |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
4eeaf51
to
6c0f661
Compare
ba56313
to
911249b
Compare
911249b
to
11dc911
Compare
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. |
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 useOpcodes.ASM9
, and adds a new methodinsnToString()
to allow debugging of single instructions.