-
Notifications
You must be signed in to change notification settings - Fork 195
Add WebAssembly features #1634
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
Add WebAssembly features #1634
Conversation
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.
So much WebAssembly! Pre-review, a first pass on names, descriptions.
A couple of repeated patterns, where I noted it a few times then stopped because needs updates throughout:
- Descriptions that do not mention WebAssembly or do not mention the feature by name. I suspect that both these should be fixed so that the description solidly stands alone. The SIMD fixed with description is a good model - names the feature and is a standalone active sentence.
- Spec URLs that do not link to the relevant portion of the spec. I checked out a couple and... sometimes it's not that clear! So maybe it's not worth forcing some of these, but should be done where it's possible.
- Feature names are not
propertyizedin descriptions - mostly this is fine since many can't really be, but should do when possible.
| description: TODO | ||
| spec: https://webassembly.github.io/spec/js-api/ | ||
| name: WebAssembly | ||
| description: WebAssembly or "wasm" is a new portable, size- and load-time-efficient format suitable for compilation to the web. |
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.
-
Should
propertyizeWebAssembly, and maybe wasm too? -
"format" sounds too generic, could be more descriptive. the wasm homepage says "binary instruction format"
| @@ -0,0 +1,6 @@ | |||
| name: WebAssembly BigInt to i64 conversion in JS | |||
| description: An extension to the WebAssembly JS API for bidrectionally converting BigInts and 64-bit WebAssembly integer values. | |||
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.
- the name makes it sound one way, but is bidirectional (unclear just how close we want to keep name to actual API name vs use-case, but thought i'd mention)
- s/bidrectionally/bidirectionally
| @@ -0,0 +1,6 @@ | |||
| name: WebAssembly BigInt to i64 conversion in JS | |||
| description: An extension to the WebAssembly JS API for bidrectionally converting BigInts and 64-bit WebAssembly integer values. | |||
| spec: https://webassembly.github.io/spec/js-api/ | |||
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.
Is there a place in the spec to link to which is more specifically about this feature?
| @@ -0,0 +1,6 @@ | |||
| name: WebAssembly bulk memory operations | |||
| description: An extension to WebAssembly adding bulk memory operations and conditional segment initialization. | |||
| spec: https://webassembly.github.io/gc/core/bikeshed/ | |||
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.
- in the spirit of "description text should stand alone", typically these mention the feature name itself, eg "The X sets Y in order to Z"
- a more specific internal link in spec would be good, if possible
| @@ -0,0 +1,16 @@ | |||
| name: WebAssembly exception handling | |||
| description: Exception handling allows code to break control flow when an exception is thrown. | |||
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.
- ditto re description standing alone, should probably mention WebAssembly specifically here
| @@ -0,0 +1,6 @@ | |||
| name: WebAssembly import & export of mutable globals | |||
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.
I suspect we should s/&/and/ here
| @@ -0,0 +1,6 @@ | |||
| name: WebAssembly import & export of mutable globals | |||
| description: An extension to WebAssembly import and export of mutable global variables. | |||
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.
I read it out loud a couple times, sounds like a missing word here
| @@ -0,0 +1,5 @@ | |||
| name: WebAssembly multi-memory | |||
| description: An extension to WebAssembly to all the use of multiple memories within a single Wasm module. | |||
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.
s/all/allow/
| @@ -1,4 +1,6 @@ | |||
| name: WebAssembly SIMD | |||
| name: WebAssembly SIMD (fixed width) | |||
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.
I was on the fence about parens in names, but there's an example in the docs which does it, so 👍🏼
| @@ -0,0 +1,5 @@ | |||
| name: WebAssembly SIMD (relaxed width) | |||
| description: The relaxed SIMD (Single Instruction Multiple Data) extension adds instructions that introduce local non-determinism (where the results of the instructions may vary based on hardware support). | |||
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.
Would read better without the long second set of parens, if possible to remix that way.
|
@petele Are you planning to come back to this PR? I'm happy to take over otherwise. |
|
@Elchi3 I prob won't have time to pick it up again until late November, so if you want to grab it, please feel free. Sorry, and thank you!! |
|
No worries Pete, I will try to drag this over the finish line in #1970. |
Worked with @tomayac our Wasm DA to break out the features into appropriate chunks.