Skip to content

Conversation

@kripken
Copy link
Member

@kripken kripken commented Mar 14, 2024

We incorrectly overrode the string operations in the interpreter's subclasses. But
string operations can be implemented in the topmost class there (as they depend on
no module state), so just implement them there, once, in a proper way.

@kripken kripken requested a review from tlively March 14, 2024 20:12
@tlively
Copy link
Member

tlively commented Mar 14, 2024

I'm confused; it looks like this PR changes all the string ops except StringEq? How could this have fixed behavior for StringEq?

@kripken
Copy link
Member Author

kripken commented Mar 14, 2024

It moves them into the right place. By removing the bad stringEq override in the subclass, the good code in the parent now works properly.

}
Flow visitStringEncode(StringEncode* curr) { return Flow(NONCONSTANT_FLOW); }
Flow visitStringConcat(StringConcat* curr) { return Flow(NONCONSTANT_FLOW); }
Flow visitStringEq(StringEq* curr) { return Flow(NONCONSTANT_FLOW); }
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the critical line for string.eq - this line overrode the parent, so we always returned "give up"...

Copy link
Member

Choose a reason for hiding this comment

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

ohhh got it.

@kripken kripken merged commit 17823cd into WebAssembly:main Mar 14, 2024
@kripken kripken deleted the string.eq.precompute branch March 14, 2024 22:06
@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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants