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: Swap side effect and check in TypedArray constructor #852

Merged
merged 1 commit into from
Mar 31, 2017

Conversation

littledan
Copy link
Member

Previously, in one of the TypedArray constructor paths, an ArrayBuffer
was checked for being detached, and then a ToIndex calculation is done.
Because ToIndex may detach the ArrayBuffer itself, this patch moves
the detached check to afterwards, so that it can include that possible
time when the ArrayBuffer may be detached.

Closes #842

Previously, in one of the TypedArray constructor paths, an ArrayBuffer
was checked for being detached, and then a ToIndex calculation is done.
Because ToIndex may detach the ArrayBuffer itself, this patch moves
the detached check to afterwards, so that it can include that possible
time when the ArrayBuffer may be detached.

Closes tc39#842
@littledan littledan added needs consensus This needs committee consensus before it can be eligible to be merged. needs test262 tests The proposal should specify how to test an implementation. Ideally via github.com/tc39/test262 normative change Affects behavior required to correctly evaluate some ECMAScript source text labels Mar 18, 2017
@littledan
Copy link
Member Author

@psmarshall Is this what you were thinking?

littledan added a commit to littledan/test262 that referenced this pull request Mar 21, 2017
…th calculation

This test is only valid with the PR in tc39/ecma262#852
@psmarshall
Copy link

Yes that looks good to me, thanks

@littledan littledan removed the needs test262 tests The proposal should specify how to test an implementation. Ideally via github.com/tc39/test262 label Mar 22, 2017
@leobalter
Copy link
Member

this reached consensus in March 22nd at the last TC39 meeting.

@leobalter leobalter removed the needs consensus This needs committee consensus before it can be eligible to be merged. label Mar 27, 2017
leobalter added a commit to tc39/test262 that referenced this pull request Mar 27, 2017
* Tests for throwing a TypeError in the TypedArray constructor on a detached buffer

Detached buffer causes an exception
- If it's already detached going into the constructor
- If the byteOffset coercion causes it to be detached

Tests are valid in ES2017

* Test that TypedArray constructor throws when detaching buffer in length calculation

This test is only valid with the PR in tc39/ecma262#852

* Rename files per review
@ljharb ljharb added the has consensus This has committee consensus. label Mar 27, 2017
@bterlson bterlson merged commit eec4f33 into tc39:master Mar 31, 2017
@anba
Copy link
Contributor

anba commented Apr 5, 2017

Two questions:

  • The proposed changes don't match the test case (test/built-ins/TypedArrays/buffer-arg-byteoffset-to-number-detachbuffer.js). Did you also mean to move step 7 (If offset modulo elementSize ≠ 0, throw a RangeError exception.) after step 9 (If IsDetachedBuffer(buffer) is true, throw a TypeError exception.), or is the test case incorrect?
  • We should also change the DataView constructor, shouldn't we? (The DataView constructor also calls ToIndex after checking for a detached array buffer.)

@littledan
Copy link
Member Author

@anba

  • Oops, this seems like a test bug.
  • Yes, that sounds like a good idea.

littledan added a commit to littledan/test262 that referenced this pull request Apr 5, 2017
Bug was reported by @anba at
tc39/ecma262#852 (comment)

Without this change, you'd expect a RangeError rather than a TypeError.
leobalter pushed a commit to tc39/test262 that referenced this pull request Apr 6, 2017
Bug was reported by @anba at
tc39/ecma262#852 (comment)

Without this change, you'd expect a RangeError rather than a TypeError.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has consensus This has committee consensus. has test262 tests normative change Affects behavior required to correctly evaluate some ECMAScript source text
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants