-
Notifications
You must be signed in to change notification settings - Fork 828
Support (module definition ...) and (module instance ...) in WAST spec tests #8058
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?
Support (module definition ...) and (module instance ...) in WAST spec tests #8058
Conversation
2480082 to
7b98b2e
Compare
tlively
left a comment
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.
A few initial comments while this is still under construction.
7b98b2e to
616165c
Compare
|
(Sorry for the force-push, I already made these changes locally before your comments) |
616165c to
e5c9ab4
Compare
| // This is not a module instance and probably a module instead. | ||
| return {}; |
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.
Let's move the lexer reset to back before the (module from the caller to here.
| // (module instance ...) | (module ...) | ||
| Result<ModuleOrInstantiation> moduleOrInstantiation(Lexer& in) { | ||
| auto reset = in; | ||
|
|
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.
Even though they both start with (module, modules and instantiations are parsed very differently and behave very differently, so I think it would make sense to treat them as different things rather than bundling them together like this. That will simplify command below by removing the need for the std::visit variant conversion.
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 mostly agree, but I chose to bundle them together since we must attempt to parse (module instance ...) before attempting (module name? ...), otherwise we'll try to parse the word instance as a name and fail rather than backtracking. Does it make sense to bundle them to make the ordering clear? I agree it's a little unfortunate that the module / module instance parsers need to be aware of each other a little bit but I don't see a way around it.
The only alternative I see is to remove the ordering dependency by letting the module (not instance) parser backtrack if it sees the word instance. That lets the ordering work either way, but that means that both parsers still need to know about each other in which case it makes sense to me to bundle them in the same function.
(module definition ...)and(module instance ...)in WAST spec tests