Skip to content
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

Update JS API to handle index types #75

Merged
merged 2 commits into from
Aug 13, 2024

Conversation

bvisness
Copy link
Collaborator

@bvisness bvisness commented Aug 2, 2024

This introduces an IndexValue typedef, which is a union of Number and BigInt, and two algorithms, IndexValueToU64 and U64ToIndexValue, which can be used to convert between IndexValue and WebAssembly's u64 type (used in the embedding spec).

It also makes several drive-by fixes and improvements.

See also #74, which handles the core spec.

Resolves #68.

This introduces an IndexValue typedef, which is a union of Number and
BigInt, and two algorithms, IndexValueToU64 and U64ToIndexValue, which
can be used to convert between IndexValue and WebAssembly's u64 type
(used in the embedding spec).

It also makes several drive-by fixes and improvements.
Copy link
Member

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

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

Thanks for working on this! I especially appreciate it because I'm not very familiar with the structure of this document myself.

lgtm % comments about perhaps splitting out the cleanups and submitting them upstream?

@@ -61,6 +62,7 @@ urlPrefix: https://tc39.github.io/ecma262/; spec: ECMASCRIPT
text: NativeError Object Structure; url: sec-nativeerror-object-structure
text: 𝔽; url: #𝔽
text: ℤ; url: #ℤ
text: mathematical value; url: #mathematical-value
Copy link
Member

Choose a reason for hiding this comment

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

This new definition, and its uses, looks like it it could be an upstream change?

@@ -1369,7 +1410,6 @@ To <dfn>initialize a Tag object</dfn> |tag| from a [=tag address=] |tagAddress|,
</div>

<div algorithm>

Copy link
Member

Choose a reason for hiding this comment

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

Revert this line maybe?

1. Let |f32| be [=nan=](n).
1. Otherwise,
1. Let |f32| be |number| rounded to the nearest representable value using IEEE 754-2008 round to nearest, ties to even mode. [[IEEE-754]]
1. Return [=f32.const=] |f32|.
1. If |type| is [=f64=],
1. Let |number| be [=?=] [$ToNumber$](|v|).
1. If |number| is **NaN**,
1. Let |n| be an implementation-defined integer such that [=canon=]<sub>64</sub> |n| < 2<sup>[=signif=](64)</sup>.
1. Let |n| be an implementation-defined integer such that [=canon=]<sub>64</sub> &leq; |n| &lt; 2<sup>[=signif=](64)</sup>.
Copy link
Member

Choose a reason for hiding this comment

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

Cleanups in this div could also be upstream change maybe?

@sbc100 sbc100 requested a review from rossberg August 5, 2024 18:49
Copy link
Member

@rossberg rossberg left a comment

Choose a reason for hiding this comment

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

LGTM modulo comment

document/js-api/index.bs Outdated Show resolved Hide resolved
Co-authored-by: Andreas Rossberg <rossberg@mpi-sws.org>
@@ -561,6 +568,8 @@ enum IndexType {
"i64",
};

typedef ([EnforceRange] unsigned long long or bigint) IndexValue;
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to file a bug as mentioned in the warning here: https://webidl.spec.whatwg.org/#dfn-union-type ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It would probably be wise, yes. Hopefully it's a quick rubber stamp and we can move on.

I'm traveling at the moment, so I would appreciate if you could open that issue.

Copy link
Member

Choose a reason for hiding this comment

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

@bvisness
Copy link
Collaborator Author

bvisness commented Aug 13, 2024

@sbc100 Are we good to merge? I don't think we really need to move the cleanups upstream since they only apply to the wasm-3.0 branch of the main spec anyway, and presumably memory64 is almost ready to move to phase 4 and be moved into the main spec repo anyway.

(As for whatwg/webidl#1426, I think we can optimistically merge this even though that issue is still open. I don't expect what we're doing to be a problem.)

@sbc100 sbc100 merged commit 1bf8e0f into WebAssembly:main Aug 13, 2024
7 checks passed
@sbc100
Copy link
Member

sbc100 commented Aug 21, 2024

Should this PR have come with test updates?

@bvisness
Copy link
Collaborator Author

Yes, probably. I didn't actually realize we had JS API tests in this repo. I'll add that to my todo list.

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.

BigInt parameters for the JS API?
3 participants