Skip to content

Migrating from getExpressionInfo to expression wrappers - #7525

Merged
kripken merged 8 commits into
WebAssembly:mainfrom
GulgDev:migrate-expression-info
Apr 23, 2025
Merged

kripken merged 8 commits into
WebAssembly:mainfrom
GulgDev:migrate-expression-info

Conversation

@GulgDev

@GulgDev GulgDev commented Apr 18, 2025

Copy link
Copy Markdown
Contributor

Replace the obsolete getExpressionInfo with expression wrapper functions.

@GulgDev

GulgDev commented Apr 18, 2025

Copy link
Copy Markdown
Contributor Author

Related discussion: #7515

@kripken kripken left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice!

Comment thread src/js/binaryen.js-post.js Outdated
@GulgDev

GulgDev commented Apr 19, 2025

Copy link
Copy Markdown
Contributor Author

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

kripken commented Apr 21, 2025

Copy link
Copy Markdown
Member

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

GulgDev commented Apr 21, 2025

Copy link
Copy Markdown
Contributor Author

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

kripken commented Apr 21, 2025

Copy link
Copy Markdown
Member

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
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