-
Notifications
You must be signed in to change notification settings - Fork 777
wast-parser: add support for '(module definition ...)' #2666
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
base: main
Are you sure you want to change the base?
Conversation
121d68d to
4c1f42a
Compare
|
@zherczeg, have you run into this in any of your recent wasm-3.0 work on wabt? Is it part of one of you in-flight PRs maybe? |
|
Is |
|
As far as I know,
|
|
No, this is not used by the GC proposal. I plan to continue working on WASM 3.0 later. |
|
@sjamesr have you verified this fix works for your external use case? If so then landing this now (even though we don't need to locally) does not seem unreasonable. |
|
Could you perhaps update the PR description mentioning why this is useful to you even though wabt itself doesn't currently use this? |
WASM v3 spec tests added the '(module definition ...)' construct to WAST. This tells the test runner that the module should not be instantiated immediately. The '(module instance ...)' construct (not yet supported in wabt) instructs the test runner to instantiate the module. This change adds an `is_definition` field to ScriptModule. The field reflects whether the `definition` keyword was encountered after the `module` keyword. When WastParser encounters a TextModule in a script command, it now returns a ScriptModule with the corresponding ScriptTextModule if the module is definition-only. This change allows wabt to parse WASM v3 spec tests that use the `(module definition ...)` construct (specifically, `memory.wast`), so that test runners built on wabt can support WASM v3 spec tests.
4c1f42a to
f645678
Compare
|
I added a paragraph to the commit and PR descriptions. I haven't actually tried it out against the v3 spec yet, so perhaps we can hold off on merging the request. I'll continue working on the assumption that we'll merge it or something like it. Just knowing that it's generally acceptable is useful, thanks! |
|
The patch LGTM. If you are OK with waiting for our own wasm-3.0 update (not sue how long that might be sadly) that might be best, since then it will get a some real testing. If you need the landed earlier thats probably fine too, just let us know, |
WASM v3 spec tests added the '(module definition ...)' construct to WAST. This tells the test runner that the module should not be instantiated immediately. The '(module instance ...)' construct (not yet supported in wabt) instructs the test runner to instantiate the module.
This change adds an
is_definitionfield to ScriptModule. The field reflects whether thedefinitionkeyword was encountered after themodulekeyword.When WastParser encounters a TextModule in a script command, it now returns a ScriptModule with the corresponding ScriptTextModule if the module is definition-only.
This change allows wabt to parse WASM v3 spec tests that use the
(module definition ...)construct (specifically,memory.wast), so that test runners built on wabt can support WASM v3 spec tests.