-
Notifications
You must be signed in to change notification settings - Fork 475
Merge js-string-builtins into wasm-3.0 #1943
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
Open
eqrion
wants to merge
74
commits into
WebAssembly:wasm-3.0
Choose a base branch
from
eqrion:js-string-builtins
base: wasm-3.0
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* Adopts builtin modules approach * Adds section of polyfilling * Adds section on feature detection * Adds cast/test builtins * Adds future extension ideas for - binding memory - utf8/wtf8 - evolving the type signatures
Update proposal
- Eliminate usage of 'builtin module' in description. This is not essential to the proposal and causes confusion around a similarly named JS proposal, which had different goals. - Clarify some minor points. - Make JS-API changes to WebIDL comprehensive. - Reword feature detection section to actually propose change to WebAssembly.validate method
- Function builtin behaviors is defined using 'create a host function' - Clarify behavior around monkey patching using standard language - Clarify edge cases around nullability - Clarify edge cases around unsigned/signed integers - Restrict 'substring' behavior to normal cases - Use wasm helpers for when wasm instructions are needed
The existing WTF-8 operation in this proposal violated one of the goals of the proposal: "don't create substantial new functionality" by introducing WTF-8 transcoding support to the web platform without prior precedent. The WTF-8 operation is removed because of this. The naming for WTF-16 operations is reworked to refer to 'charCodes' instead as that is what the JS String interface uses. We could support UTF-8 transcoding by referring to the TextEncoder/TextDecoder interfaces, so this commit adds support for that.
Explain that users should validate modules that deliberately produce link errors to test for support for particular builtins.
This commit adds a basic suite of tests for the js-string-builtins. This is done by defining a polyfill module matching the overview, and then comparing the host provided builtins against the polyfill on representative inputs.
- Syntax highlighting - Added links - Code font - Consistency - Grammer fixes
Needs more detail to properly integrate with GC array ops.
This avoids having to refer to actual Wasm instructions, since after all this is a host function.
Equals specifically allows null inputs. Update the JS API tests and the polyfill to match.
I'll let @Ms2ger review thia, since it only touches the JS specs. |
Thanks - no obvious issues jump out at me while skimming; I'll try to take a slightly more thorough look by next week. Should the tests still be marked as tentative? |
Good catch, updated. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There are no substantive open issues on the js-string-builtins repo right now, this has shipped in two browsers now (Chrome and Firefox), and so I'd like to start the process of merging this into the spec. I originally based this off the wasm-3.0 branch, so that's what I'm basing this PR onto.
From my read of the process document, js-string-builtins needs to be phase 5 before merging into the spec repo. So I've filed an agenda item for advancing js-string-builtins to phase 5 here. I think we could start review of this before then to catch any potential issues that may need to be discussed with the WG. If not, I'll just leave this here until after the next WG meeting.
cc' @rossberg @Ms2ger as editors.