-
Notifications
You must be signed in to change notification settings - Fork 21
Conversation
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.
9638e49
to
d1e3c03
Compare
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.
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 |
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.
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> | |||
|
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.
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> ≤ |n| < 2<sup>[=signif=](64)</sup>. |
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.
Cleanups in this div could also be upstream change maybe?
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.
LGTM modulo comment
Co-authored-by: Andreas Rossberg <rossberg@mpi-sws.org>
@@ -561,6 +568,8 @@ enum IndexType { | |||
"i64", | |||
}; | |||
|
|||
typedef ([EnforceRange] unsigned long long or bigint) IndexValue; |
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.
Do we need to file a bug as mentioned in the warning here: https://webidl.spec.whatwg.org/#dfn-union-type ?
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.
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.
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.
@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.) |
Should this PR have come with test updates? |
Yes, probably. I didn't actually realize we had JS API tests in this repo. I'll add that to my todo list. |
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.