Skip to content

Conversation

@EngoDev
Copy link
Contributor

@EngoDev EngoDev commented Oct 7, 2025

Split from #298

Copy link
Member

@tschneidereit tschneidereit left a comment

Choose a reason for hiding this comment

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

Thank you for splitting this off! I left a bunch of comments, addressing which I think should make this a very small diff indeed. I might be wrong about some assumptions though, as I didn't attempt any of these changes myself.

}
}
builder.inject_at(
// Inject adjustments after the located instruction: add delta and add arg index
Copy link
Member

Choose a reason for hiding this comment

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

Why doesn't the inject_at approach work here anymore? Maybe you just couldn't find it anymore because it moved into the InjectAt trait? If you change the use wirm::opcode::Inject; statement to use wirm::opcode::{Inject, InjectAt}; I think the original code should work as before.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I totally missed the InjectAt, added it back but the other code is necessary to set the instruction as it can't be done in the loop for the reason in the previous comment

@EngoDev EngoDev requested a review from tschneidereit October 7, 2025 18:29
@EngoDev EngoDev requested a review from tschneidereit October 10, 2025 17:17
Copy link
Member

@tschneidereit tschneidereit left a comment

Choose a reason for hiding this comment

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

Thank you for this, and apologies for the long delay in this last review!

@tschneidereit tschneidereit merged commit e1d5020 into bytecodealliance:main Oct 24, 2025
12 checks passed
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.

3 participants