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
feat: Add toString method for V128 type #2137
base: main
Are you sure you want to change the base?
Conversation
MaxGraey
commented
Nov 11, 2021
- I've read the contributing guidelines
- I've added my name and email to the NOTICE file
|
I am not quite sure what's the best textual notation here, but in the absence of a better idea I guess it's fine to merge this for now :) |
| @@ -1,4 +1,51 @@ | |||
| /** Vector abstraction. */ | |||
| @final @unmanaged | |||
| export abstract class V128 { | |||
| toString(this: v128, radix: i32 = 0): String { | |||
| if (radix == 0 || radix == 10) { | |||
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.
What's the purpose of this branch btw? Looks like if radix can be derived statically, we'd get a little less code, while if it can't, we'd get double the code.
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 main diff is 10 radix uses extract_lane_s while rest of radixes use extract_lane_u
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.
Ah, I see. Feels a little inconsistent, though. Is this behavior based on a particular assumption? From Binaryen or so?
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 this case it is a rather temporary solution, because we have no separated types like i8x16, u32x4 and all these types are associated with a general v128 proto. So to get something like pretty printing for the standard radix = 10 I use the signed version abut for radix = 16 and others - unsigned, which is usually expected.
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.
Perhaps conceptionally a v128 is similar to an ArrayBuffer containing fixed length untyped data. ArrayBuffer's toString is just [object ArrayBuffer] but it has views like Uint8Array to make sense of the values separately, while v128 currently has no such views. If we'd want to align with JS's precedent (in the future), an untyped 128-bit vector's toString could be [v128] while a future i8x16 could yield 0,127,-1,... like an Int8Array. Conservatively, it might make sense to be careful about v128's toString then, since changing it again is, for those who begin to rely on the temporary solution, a breaking change. As such I'd kinda prefer a conservative solution like [v128] I think, hmm. Wdyt?
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 basic idea is print i8x16(0, -127, ...).toString() as 0 -127 ... and i8x16(0, -127, ...).toString(16) as 0x00 0x81 ... instead 0x00 -0x7f .... But I'm ok if we always will print values as signed 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.
If there'd only be i8x16 that would probably be ok I guess, but it doesn't work so well for other types, say i32x4(0, 255, ...).toString() is quite surprising. To avoid backwards incompatible changes, an approach could be:
- Add a
V128#toStringhere but make it independent of the representation, say returning[v128]so we won't have to change it again in the future. - Add
i8x16,i32x4etc. types later which implement their own#toStringdoing the correct thing, say modeled afterInt8Array#toString. Make the new types implicitly castable to and fromv128.
My primary concern is the backwards incompatibility aspect that seems to result from being too eager to early, and that it is avoidable.
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 prefer point 2. But it required significant efforts, which is out of scope of current PR.
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.
Right, hence suggesting to do 1. here and 2. later to be safe.
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 this PR already add V128#toString