-
Notifications
You must be signed in to change notification settings - Fork 779
Add missing ids to getExpressionInfo
and fix existing bugs
#7507
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
Conversation
Yes, that does seem redundant... Implementing getExpressionInfo using the wrappers might be good. I think we can just return the corresponding wrapper - it's an API change but seems strictly better, and low risk. |
If we return the wrapper instead of an info object this would not only break some code (they are not strictly equal), but that would also mean that info objects now mutate the state of the expression. Is this change wanted? Maybe it would be better to re-define all properties of the wrapper on a new object, with the needed changes to maintain compatibility with the previous API. Also, should |
Yes, being able to get info for an expression of unknown class was probably the main motivation for getExpressionInfo. I agree we can deprecate it if we add such a method that creates a wrapper. Seems safer than a breaking change, good point. |
Should this be implemented in a separate PR or is it ok to repurpose this one? |
Maybe leave that for a later PR? The code in this PR looks good to land, unless it is incomplete somehow? |
It is complete, but I am working on changing implicit |
Sounds good, let me know when it's ready here. |
…sistencies in `getExpressionInfo`
@kripken Done! I've also fixed some bugs that appeared after updating the tests. |
return { | ||
'id': id, | ||
'type': type, | ||
'name': UTF8ToString(Module['_BinaryenBlockGetName'](expr)), | ||
'name': name ? UTF8ToString(name) : null, |
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.
Perhaps we could have a UTF8ToStringOrNull
helper for this pattern, but we can consider that separately.
This PR adds cases for GC expressions in
getExpressionInfo
and updatesCallIndirectId
(fixes #7490),TryId
. In addition, it also adds tests forgetExpressionInfo
results, which were not checked before.Also,
getExpressionInfo
is very similar to the expression wrappers, so maybe we should modify this function to just return a copy of the wrapper? If we do so, should we either return the corresponding wrapper or return an object which has all the required properties of the wrapper (should they be value properties or getter properties?)?