Skip to content

Migrating from getExpressionInfo to expression wrappers #7525

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

Merged
merged 8 commits into from
Apr 23, 2025

Conversation

GulgDev
Copy link
Contributor

@GulgDev GulgDev commented Apr 18, 2025

Replace the obsolete getExpressionInfo with expression wrapper functions.

@GulgDev
Copy link
Contributor Author

GulgDev commented Apr 18, 2025

Related discussion: #7515

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.

Nice!

@GulgDev
Copy link
Contributor Author

GulgDev commented Apr 19, 2025

Maybe we can make Expression constructor return the specific instance? Can we just make the instruction-building functions return expression wrappers? That shouldn't be a breaking change, as expression wrappers can be implicitly converted to Wasm pointers via valueOf (e.g. +expr or expr | 0).

@kripken
Copy link
Member

kripken commented Apr 21, 2025

Maybe we can make Expression constructor return the specific instance?

Sorry, what do you mean here? What type of code can construct something with only Expression?

@GulgDev
Copy link
Contributor Author

GulgDev commented Apr 21, 2025

I mean that the Expression constructor could possibly act as wrapExpression.

// Base class of all expression wrappers
/** @constructor */
function Expression(expr) {
if (!expr) throw Error("expression reference must not be null");
this[thisPtr] = expr;
}

@kripken
Copy link
Member

kripken commented Apr 21, 2025

I see, thanks. Yes, I guess it could. Initially it felt slightly odd to me, maybe because in C++ Expression(..) returns an Expression, not the more specific subclass... but in JS it seems ok, and I can't think of anything simpler. Let's go with that.

@GulgDev GulgDev marked this pull request as ready for review April 23, 2025 12:35
@GulgDev GulgDev requested a review from kripken April 23, 2025 12:35
@kripken kripken merged commit 637326b into WebAssembly:main Apr 23, 2025
14 checks passed
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