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

feat: Add toString method for V128 type #2137

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

Copy link
Member

@MaxGraey MaxGraey commented Nov 11, 2021

  • I've read the contributing guidelines
  • I've added my name and email to the NOTICE file

@MaxGraey MaxGraey requested a review from dcodeIO Nov 11, 2021
std/assembly/index.d.ts Outdated Show resolved Hide resolved
@dcodeIO
Copy link
Member

@dcodeIO dcodeIO commented Dec 2, 2021

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 :)

@MaxGraey MaxGraey requested a review from dcodeIO Apr 10, 2022
@@ -1,4 +1,51 @@
/** Vector abstraction. */
@final @unmanaged
export abstract class V128 {
toString(this: v128, radix: i32 = 0): String {
if (radix == 0 || radix == 10) {
Copy link
Member

@dcodeIO dcodeIO Apr 10, 2022

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.

Copy link
Member Author

@MaxGraey MaxGraey Apr 10, 2022

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

Copy link
Member

@dcodeIO dcodeIO Apr 10, 2022

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?

Copy link
Member Author

@MaxGraey MaxGraey Apr 10, 2022

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.

Copy link
Member

@dcodeIO dcodeIO Apr 11, 2022

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?

Copy link
Member Author

@MaxGraey MaxGraey Apr 11, 2022

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

Copy link
Member

@dcodeIO dcodeIO Apr 11, 2022

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:

  1. Add a V128#toString here but make it independent of the representation, say returning [v128] so we won't have to change it again in the future.
  2. Add i8x16, i32x4 etc. types later which implement their own #toString doing the correct thing, say modeled after Int8Array#toString. Make the new types implicitly castable to and from v128.

My primary concern is the backwards incompatibility aspect that seems to result from being too eager to early, and that it is avoidable.

Copy link
Member Author

@MaxGraey MaxGraey Apr 11, 2022

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.

Copy link
Member

@dcodeIO dcodeIO Apr 11, 2022

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.

Copy link
Member Author

@MaxGraey MaxGraey Apr 11, 2022

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

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.

None yet

2 participants