-
Notifications
You must be signed in to change notification settings - Fork 38
String builtins merge, passing builtinSetNames and stringConstant #115
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
9ccf7db to
0709449
Compare
|
@guybedford Overall LGTM. I'm uncertain about the string constants import though. I think we'd be unhappy if we were always stuck with My understanding is that not picking a name for string constants now doesn't preclude us from picking one later? So could we continue to defer the question? I know I've not made any progress on compact-imports recently, but I do hope to pick it up soon. I think if that moves to phase 2 we'll have more clarity here. |
|
@eqrion thanks for taking a look here. I wonder if compact imports could be framed as an optimization that would result in a single instance import like That is, can we make this decision in anticipation of compact imports that Would be good to discuss this further. Yes we can wait on this decision here, and still be able to decide later. But string constants are also an important piece of the string builtins story that we shouldn't delay on unnecessarily either. |
|
Just my 2c on |
| 1. Note: When integrating with the JS String Builtins proposal, |builtinSetNames| should be passed in the following step as « "js-string" » and |importedStringModule| as null. | ||
| 1. [=Construct a WebAssembly module object=] from |module| and |bytes|, and let |module| be the result. | ||
| 1. Let |builtinSetNames| be « "js-string" ». | ||
| 1. Let |importedStringModule| be "wasm:js/string-constants". |
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.
As mentioned on the other bug. I'm also happy with wasm:js/string/constants. I didn't think of it as a submodule before but agree that's a good way to think of it.
With the spec now merged to the latest, we can explicitly integrate with the JS String Builtins proposal, passing the
builtinSetNamesandstringConstantparameters to module construction.This explicitly now passes
« "js-string" »for the builtinset.For the string constant, we had a bikeshed discussion in #118, which led to the conclusion of
wasm:js/string-constants, which is specified here as well.// cc @eqrion