bakkot editor review
Closed this issue · 3 comments
Normative:
-
transferToImmutabledoes not appear to actually detach the original buffer. The new code inArrayBufferCopyAndDetachearly exits before it would reach the call toDetachArrayBuffer. 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).
#36 - There's an open normative question in
ArrayBuffer.prototype.sliceToImmutablewhich should be resolved prior to stage 2.7.
#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.
#37
Editorial (can be resolved later):
- "preserveResizability" is not a reasonable name for a variable which can take the value "immutable". Call it "mode" or something.
#35 -
AllocateImmutableArrayBuffertakes a constructor argument which is asserted to be%ArrayBuffer%. It should just hardcode that rather than taking an argument.
@bakkot for our own convenience making progress on your suggestions, I've turned your bullets in check boxes.
AllocateImmutableArrayBuffertakes 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).
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.