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

Normative: move 'into' methods onto prototype and rename #45

Merged
merged 1 commit into from
Feb 7, 2024
Merged

Conversation

bakkot
Copy link
Collaborator

@bakkot bakkot commented Jan 25, 2024

Fixes #41. In Matrix (x, x) people mildly favored having these on the prototype, and while there wasn't universal agreement on the names no one objected strongly to setFromBase64, which was my favorite.

I'm going to leave this open for the moment to get more eyes on it.

cc @anba

Also cc @annevk since this breaks some symmetry with encodeInto (but, I think, probably not in a way which will be confusing for users - no more than having a toBase64 prototype method vs (new TextDecoder).decode, anyway).

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Changes are 100% correct, assuming the base spec is correct and the change ends up with consensus :-)

<emu-alg>
1. If _string_ is not a String, throw a *TypeError* exception.
1. Let _into_ be the *this* value.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
1. Let _into_ be the *this* value.
1. Let _into_ be the *this* value.
1. Perform ? ValidateTypedArray(_into_, ~seq-cst~).

Here and below to match other TypedArray.prototype built-ins.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The closest such built-in is TypedArray.prototype.set, and it, like this method, initially checks only the presence of the internal slot on this, and then does the potentially side-effecting handling of arguments, and only after that does the IsTypedArrayOutOfBounds check.

That's what I'm doing here.

Copy link
Contributor

Choose a reason for hiding this comment

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

TypedArray.prototype.set (and TypedArray.prototype.subarray) are both legacy Khronos-based functions, so I don't think it's useful/necessary to copy the same approach in new methods. Instead it's preferable to use the same validation sequence which is present in all newer TypedArray built-ins.

Copy link
Contributor

Choose a reason for hiding this comment

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

(Actually it'd great if we could change TypedArray.prototype.{set,subarray} to call ValidateTypedArray at the start, because it's kind of annoying that subarray can observe the original [[ByteOffset]] when the underlying buffer is detached, see https://bugzilla.mozilla.org/show_bug.cgi?id=1291003 and https://bugzilla.mozilla.org/show_bug.cgi?id=1840991. Being able to read [[ByteOffset]] from detached/out-of-bounds typed arrays also requires extra JIT support in SpiderMonkey, where subarray is a self-hosted built-in: https://phabricator.services.mozilla.com/D200658.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You want to get the length (and validate non-detachedness) after side-effects happen anyway. Checking up-front would mean duplicating that work.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

cc @syg for thoughts.

Copy link

@syg syg Feb 6, 2024

Choose a reason for hiding this comment

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

You want to get the length (and validate non-detachedness) after side-effects happen anyway. Checking up-front would mean duplicating that work.

I agree with this.

I'm leaning towards breaking with consistency. If we do that, the most desirable processing order for TypedArray builtins are to first process all the parameters, then do a ValidateTypedArray on this. After that point, with the exception of built-ins that call user-provided callbacks in a loop (like map or something), there should be no more user code being called.

If for whatever reason committee is not amenable to breaking with consistency, the ValidateTypedArray on this, then arguments, then ValidateTypedArray again order is wasteful. And given that maximal consistency is not possible since the builtins are already inconsistent, I see no compelling reason to be wasteful.

@sffc
Copy link

sffc commented Feb 7, 2024

Reducing the number of positional arguments is good, and the prototype method reduces the number of positional arguments.

@bakkot
Copy link
Collaborator Author

bakkot commented Feb 7, 2024

Got consensus for this change.

@bakkot bakkot merged commit 826e009 into main Feb 7, 2024
1 check passed
@bakkot bakkot deleted the move-into branch February 7, 2024 23:44
@bakkot
Copy link
Collaborator Author

bakkot commented Feb 8, 2024

Test262 tests have been updated.

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.

Placement and name of "from{Base64,Hex}Into"
5 participants