Skip to content

Conversation

XMadrid
Copy link
Contributor

@XMadrid XMadrid commented May 8, 2024

fix #6557

@XMadrid
Copy link
Contributor Author

XMadrid commented May 8, 2024

Should I add a test case using CFGWalker with single-thread in test/gtest/ ?

@kripken
Copy link
Member

kripken commented May 8, 2024

Code here looks good.

About a test, if it's simple to make one then it sounds like a good idea. Perhaps the test can do something like

Walker walker; walker.walk(); Walker fresh; assert(walker == fresh); // walker should have no changes left

But I'm not sure if that can work. I'd be ok landing this without a test if testing isn't easy.

@XMadrid
Copy link
Contributor Author

XMadrid commented May 9, 2024

Code here looks good.

About a test, if it's simple to make one then it sounds like a good idea. Perhaps the test can do something like

Walker walker; walker.walk(); Walker fresh; assert(walker == fresh); // walker should have no changes left

But I'm not sure if that can work. I'd be ok landing this without a test if testing isn't easy.

It's not easy to do such an assertion, I agree that we can merge this without a test.

Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

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

Sounds good, let's land this as-is. Thanks for the PR!

@kripken kripken merged commit 712ad9d into WebAssembly:main May 9, 2024
@gkdn gkdn mentioned this pull request Aug 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

2 participants