Updates to TypedArrays to match spec. #4442

Closed
wants to merge 7 commits into
from

Projects

None yet

3 participants

@deanm

These again are a few effective reverts of previous changes by Mikael Bourges-Sevenier to the TypedArrays code. I believe it was caused by a misreading of the spec, where get() and set() methods are defined as getters/setters ([] and []= operators). By removing these and updating a few exception strings the TypedArray implementation now fully passes the following related WebKit tests:

array-buffer-crash.html
array-buffer-view-crash-when-reassigned.html
array-buffer-view-crash.html
array-constructor.html
array-get-and-set-method-removal.html
array-get-out-of-bounds.html
array-override-set.html
array-set-invalid-arguments.html
array-set-out-of-bounds.html
array-set-with-offset.html
array-setters.html
array-unit-tests.html
dfg-float32array.js
dfg-int16array.js
dfg-int32array-overflow-values.js
dfg-int32array.js
dfg-int8array.js
dfg-uint16array.js
dfg-uint32array-overflow-values.js
dfg-uint32array.js
dfg-uint8array.js
dfg-uint8clampedarray.js

deanm added some commits Dec 7, 2012
@deanm deanm Export TypedArrays SizeOfArrayElementForType().
Althought it is not used externally by node, it is needed by upstream and Plask.

This effectively reverts:

    commit 1444801
    Author: Aaron Jacobs <jacobsa@google.com>
    Date:   Thu Mar 15 13:26:35 2012 +1100

        typed arrays: unexport SizeOfArrayElementForType()

        It isn't used anywhere else, so made it an implementation detail in
        v8_typed_array.cc.
4b528a2
@deanm deanm Fix missing type in TypedArrays SizeOfArrayElementForType.
When Mikael Bourges-Sevenier added support for Uint8ClampedArray, the new type
was not added to SizeOfArrayElementForType.
c106414
@deanm deanm Merge branch 'master' of github.com:deanm/node c93055d
@deanm deanm Remove TypedArrays get().
This seems to have been added as a result of misreading the spec, there is no
get() method, only a getter (which the spec names get()), but this is actually
the [] operator.  There are many webkit tests that explicitly test for the
fact that the get() method is abscent.  Remove it to conform to the spec.
d27185c
@deanm deanm Remove TypedArrays set(index, val) and match WebKit exception strings.
It seems that like get(), set(index, val) was added as a misreading of the spec.
There are only two set() methods defined in the spec:
    void set(TypedArray array, optional unsigned long offset)
    void set(type[] array, optional unsigned long offset)
The set(index, val) is handled by the []= operator.
Additionally updated a few exception error strings to match WebKit.
6b44b05
@TooTallNate

Are the last 2 commits the relevant ones?

@deanm

Yes, sorry, I can make a clean branch if it helps. There are now 4 relevant commits (although it could be broken into 2 pull requests if you want). The ones of interest are removing get() and set(), and then the other are some cast changes (no code change) and some changes to exception messages to match WebKit. This is because I am running the WebKit tests. Once these changes are in (which gets us pretty much passing all of the tests), there is one more bugfix I have for when we are compiled 32-bit, and then I will make sure the tests pass for both 32 and 64-bit version of Node.

@bnoordhuis
Node.js Foundation member

I'm afraid the patches no longer apply. Dean, can you rebase them against master and submit them as a new PR? I promise I'll review them quickly. :-)

@bnoordhuis bnoordhuis closed this Jan 8, 2013
@joaocgreis joaocgreis pushed a commit to janeasystems/node-v0.x-archive that referenced this pull request Feb 12, 2016
@santigimeno santigimeno test: fix child-process-fork-regr-gh-2847 again
Windows is still sometimes failing with ECONNRESET. Bring back the
handling of this error as was initially introduced in PR #4442.

PR-URL: nodejs/node#5179
Reviewed-By: Rich Trott <rtrott@gmail.com>
Fixes: nodejs/node#3635
24667ee
@richardlau richardlau pushed a commit to ibmruntimes/node that referenced this pull request Feb 18, 2016
@santigimeno santigimeno test: fix child-process-fork-regr-gh-2847 again
Windows is still sometimes failing with ECONNRESET. Bring back the
handling of this error as was initially introduced in PR #4442.

PR-URL: nodejs/node#5179
Reviewed-By: Rich Trott <rtrott@gmail.com>
Fixes: nodejs/node#3635
424c569
@richardlau richardlau pushed a commit to ibmruntimes/node that referenced this pull request Feb 18, 2016
@santigimeno santigimeno test: fix child-process-fork-regr-gh-2847 again
Windows is still sometimes failing with ECONNRESET. Bring back the
handling of this error as was initially introduced in PR #4442.

PR-URL: nodejs/node#5179
Reviewed-By: Rich Trott <rtrott@gmail.com>
Fixes: nodejs/node#3635
7c88410
@richardlau richardlau pushed a commit to ibmruntimes/node that referenced this pull request Feb 18, 2016
@santigimeno santigimeno test: fix child-process-fork-regr-gh-2847 again
Windows is still sometimes failing with ECONNRESET. Bring back the
handling of this error as was initially introduced in PR #4442.

PR-URL: nodejs/node#5179
Reviewed-By: Rich Trott <rtrott@gmail.com>
Fixes: nodejs/node#3635
b639c33
@richardlau richardlau pushed a commit to ibmruntimes/node that referenced this pull request Mar 9, 2016
@santigimeno santigimeno test: fix child-process-fork-regr-gh-2847 again
Windows is still sometimes failing with ECONNRESET. Bring back the
handling of this error as was initially introduced in PR #4442.

PR-URL: nodejs/node#5179
Reviewed-By: Rich Trott <rtrott@gmail.com>
Fixes: nodejs/node#3635
972c063
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment