Skip to content

bakkot editor review #31

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

Closed
4 of 5 tasks
bakkot opened this issue Feb 10, 2025 · 3 comments
Closed
4 of 5 tasks

bakkot editor review #31

bakkot opened this issue Feb 10, 2025 · 3 comments

Comments

@bakkot
Copy link

bakkot commented Feb 10, 2025

Normative:

  • transferToImmutable does not appear to actually detach the original buffer. The new code in ArrayBufferCopyAndDetach early exits before it would reach the call to DetachArrayBuffer. It also early exits before checking the [[ArrayBufferDetachKey]], which must be done when detaching since some host specs define ArrayBuffers which cannot be detached by normal means (for example WebAssembly and WebGPU).
    Normative: Fix ArrayBufferCopyAndDetach #36
  • There's an open normative question in ArrayBuffer.prototype.sliceToImmutable which should be resolved prior to stage 2.7.
    Editorial: Replace open TODO with an explanatory note #34
  • In SetViewValue, mutability of the target is checked after coercing arguments. Usually we do validation of the receiver first. We can't do all the validation up-front here because side effects from coercing the argument can detach the buffer, but they can't cause it to become immutable, so we could reasonably check mutability earlier, i.e., before coercing the arguments. I'd default to doing so, although I'm open to arguments that we should not check it early.
    Normative: Validate DataView buffer mutability before argument coercion #37

Editorial (can be resolved later):

@erights
Copy link
Collaborator

erights commented Feb 11, 2025

@bakkot for our own convenience making progress on your suggestions, I've turned your bullets in check boxes.

gibson042 added a commit to gibson042/tc39-proposal-immutable-arraybuffer that referenced this issue Feb 15, 2025
gibson042 added a commit to gibson042/tc39-proposal-immutable-arraybuffer that referenced this issue Feb 15, 2025
gibson042 added a commit that referenced this issue Feb 15, 2025
gibson042 added a commit to gibson042/tc39-proposal-immutable-arraybuffer that referenced this issue Feb 15, 2025
Ref tc39#31

* Always respect [[ArrayBufferDetachKey]].
* Always detach.
gibson042 added a commit that referenced this issue Feb 15, 2025
gibson042 added a commit to gibson042/tc39-proposal-immutable-arraybuffer that referenced this issue Feb 15, 2025
@gibson042
Copy link
Collaborator

AllocateImmutableArrayBuffer takes a constructor argument which is asserted to be %ArrayBuffer%. It should just hardcode that rather than taking an argument.

@bakkot All concerns have been addressed (see new links above) except for this one, which I have declined because I want the spec to preserve parallel arguments for AllocateArrayBuffer ( constructor, byteLength [ , maxByteLength ] ) vs. AllocateImmutableArrayBuffer ( constructor, byteLength, fromBlock, fromIndex, count ). If that is going to change, it can come later (or alternatively, we could drop the assertion—but I'd rather not, because I find it clarifying).

@ljharb ljharb mentioned this issue Feb 15, 2025
8 tasks
@bakkot
Copy link
Author

bakkot commented Feb 17, 2025

Matching AllocateArrayBuffer is less important to me than ensuring readers are not misled into thinking AllocateImmutableArrayBuffer can create non-base ArrayBuffers, but it's fine to make that change later. Other changes LGTM.

You can consider my review complete.

@bakkot bakkot closed this as completed Feb 17, 2025
@erights erights mentioned this issue Feb 17, 2025
44 tasks
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

No branches or pull requests

3 participants