Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
660863003:
Implement .of() on typed arrays (Closed)
DescriptionImplement .of() on typed arrays
BUG=v8:3578
LOG=Y
Patch Set 1 #
Total comments: 15
Patch Set 2 : Update after review #
Total comments: 2
Patch Set 3 : Fixed nits #
Messages
Total messages: 12 (3 generated)
aperez@igalia.com changed reviewers: + wingo@chromium.org
wingo@chromium.org changed reviewers: + wingo@igalia.com - wingo@chromium.org
spec link here: http://people.mozilla.org/~jorendorff/es6-draft.html#sec-%typedarray%.of In the future please send a link with these; section numbers are too variable
https://codereview.chromium.org/660863003/diff/1/src/harmony-typedarray.js File src/harmony-typedarray.js (right): https://codereview.chromium.org/660863003/diff/1/src/harmony-typedarray.js#ne... src/harmony-typedarray.js:66: if (!IS_SPEC_FUNCTION(this)) { what does this case check? wouldn't this be caught by the new this(len) call below? https://codereview.chromium.org/660863003/diff/1/src/harmony-typedarray.js#ne... src/harmony-typedarray.js:78: if (!%IsTypedArray(array)) { This isn't in the spec. https://codereview.chromium.org/660863003/diff/1/src/harmony-typedarray.js#ne... src/harmony-typedarray.js:95: "of", NAMEOf This outer function name is a bit misleading -- we aren't extending the prototype, we're extending the global. Also it seems that "of" should be non-writable and non-configurable as well, as per the defaults in http://people.mozilla.org/~jorendorff/es6-draft.html#table-4 https://codereview.chromium.org/660863003/diff/1/test/mjsunit/harmony/typedar... File test/mjsunit/harmony/typedarrays-of.js (right): https://codereview.chromium.org/660863003/diff/1/test/mjsunit/harmony/typedar... test/mjsunit/harmony/typedarrays-of.js:24: assertEquals(constructor.of.length, 0); remove this check, it's done below https://codereview.chromium.org/660863003/diff/1/test/mjsunit/harmony/typedar... test/mjsunit/harmony/typedarrays-of.js:33: assertEquals(true, !!a[3]); go ahead and inline these into the branches of the if below https://codereview.chromium.org/660863003/diff/1/test/mjsunit/harmony/typedar... test/mjsunit/harmony/typedarrays-of.js:86: Object.defineProperty(constructor.prototype, "0", {set: function(v) {status = "FAIL 1"}}); 80 columns https://codereview.chromium.org/660863003/diff/1/test/mjsunit/harmony/typedar... test/mjsunit/harmony/typedarrays-of.js:87: assertEquals(1, constructor.of(1)[0], 1); does this test what you mean to test? not sure, as the 0 is on the prototype, and the Put might not work if the getter is on the instance. i can't recall. https://codereview.chromium.org/660863003/diff/1/test/mjsunit/harmony/typedar... test/mjsunit/harmony/typedarrays-of.js:90: // Array.of calls a "length" setter if one is present. This doesn't appear in the spec AFAICS. https://codereview.chromium.org/660863003/diff/1/test/mjsunit/harmony/typedar... test/mjsunit/harmony/typedarrays-of.js:123: Object.defineProperty(Empty.prototype, "length", {get: function() { return 0; }}); 80 https://codereview.chromium.org/660863003/diff/1/test/mjsunit/harmony/typedar... test/mjsunit/harmony/typedarrays-of.js:135: assertEquals(desc.configurable, true); should be false i think https://codereview.chromium.org/660863003/diff/1/test/mjsunit/harmony/typedar... test/mjsunit/harmony/typedarrays-of.js:137: assertEquals(desc.writable, true); should be false https://codereview.chromium.org/660863003/diff/1/test/mjsunit/harmony/typedar... test/mjsunit/harmony/typedarrays-of.js:139: assertThrows(function() { new constructor.of() }, TypeError); // not a constructor 80 https://codereview.chromium.org/660863003/diff/1/test/mjsunit/harmony/typedar... test/mjsunit/harmony/typedarrays-of.js:142: [undefined, null, false, "cow"].forEach(function(val) { cleaner to do for (var x of [undefined, null, false, "cow"]) foo() https://codereview.chromium.org/660863003/diff/1/test/mjsunit/harmony/typedar... test/mjsunit/harmony/typedarrays-of.js:143: assertEquals(true, constructor.of(val) instanceof constructor); this doesn't appear to test what you intend to test -- you want contstructor.of.call(val) i think https://codereview.chromium.org/660863003/diff/1/test/mjsunit/harmony/typedar... test/mjsunit/harmony/typedarrays-of.js:147: for (var i = 0; i < typedArrayConstructors.length; i++) { for (var constructor of typedArrayConstructors) { }
On 2014/10/21 15:23:13, wingo wrote: > https://codereview.chromium.org/660863003/diff/1/src/harmony-typedarray.js > File src/harmony-typedarray.js (right): > > https://codereview.chromium.org/660863003/diff/1/src/harmony-typedarray.js#ne... > src/harmony-typedarray.js:66: if (!IS_SPEC_FUNCTION(this)) { > what does this case check? wouldn't this be caught by the new this(len) call > below? Yes, indeed. > https://codereview.chromium.org/660863003/diff/1/src/harmony-typedarray.js#ne... > src/harmony-typedarray.js:78: if (!%IsTypedArray(array)) { > This isn't in the spec. Acknowledged. > https://codereview.chromium.org/660863003/diff/1/src/harmony-typedarray.js#ne... > src/harmony-typedarray.js:95: "of", NAMEOf > This outer function name is a bit misleading -- we aren't extending the > prototype, we're extending the global. Well, for Array the corresponding is called “ArrayOf()” (as seen in “src/harmony-array.js”). Once the macro expanded, it creates functions named “Int8ArrayOf”, “Int16ArrayOf”, and so on. So the final result follows the same naming scheme as the existing “ArrayOf()”... I can split this in two functions, one that extends the constructors and other that extends the prototypes. Would that make sense to you? > Also it seems that "of" should be non-writable and non-configurable as well, as > per the defaults in http://people.mozilla.org/~jorendorff/es6-draft.html#table-4 Right. As far as this goes, a bunch of other built-in functions are being attached to constructors and prototypes with “DONT_ENUM” only, which probably means that at some point all of them should be changed to use “DONT_ENUM | READ_ONLY | DONT_DELETE”. Am I right? > https://codereview.chromium.org/660863003/diff/1/test/mjsunit/harmony/typedar... > File test/mjsunit/harmony/typedarrays-of.js (right): > > https://codereview.chromium.org/660863003/diff/1/test/mjsunit/harmony/typedar... > test/mjsunit/harmony/typedarrays-of.js:24: assertEquals(constructor.of.length, > 0); > remove this check, it's done below Ack. > https://codereview.chromium.org/660863003/diff/1/test/mjsunit/harmony/typedar... > test/mjsunit/harmony/typedarrays-of.js:33: assertEquals(true, !!a[3]); > go ahead and inline these into the branches of the if below Ack. > https://codereview.chromium.org/660863003/diff/1/test/mjsunit/harmony/typedar... > test/mjsunit/harmony/typedarrays-of.js:86: > Object.defineProperty(constructor.prototype, "0", {set: function(v) {status = > "FAIL 1"}}); > 80 columns > > https://codereview.chromium.org/660863003/diff/1/test/mjsunit/harmony/typedar... > test/mjsunit/harmony/typedarrays-of.js:87: assertEquals(1, constructor.of(1)[0], > 1); > does this test what you mean to test? not sure, as the 0 is on the prototype, > and the Put might not work if the getter is on the instance. i can't recall. > > https://codereview.chromium.org/660863003/diff/1/test/mjsunit/harmony/typedar... > test/mjsunit/harmony/typedarrays-of.js:90: // Array.of calls a "length" setter > if one is present. > This doesn't appear in the spec AFAICS. > https://codereview.chromium.org/660863003/diff/1/test/mjsunit/harmony/typedar... > test/mjsunit/harmony/typedarrays-of.js:135: assertEquals(desc.configurable, > true); > should be false i think Yes, as per the table with the defaults you linked. > https://codereview.chromium.org/660863003/diff/1/test/mjsunit/harmony/typedar... > test/mjsunit/harmony/typedarrays-of.js:137: assertEquals(desc.writable, true); > should be false Ditto. > https://codereview.chromium.org/660863003/diff/1/test/mjsunit/harmony/typedar... > test/mjsunit/harmony/typedarrays-of.js:142: [undefined, null, false, > "cow"].forEach(function(val) { > cleaner to do > > for (var x of [undefined, null, false, "cow"]) > foo() Okay. > https://codereview.chromium.org/660863003/diff/1/test/mjsunit/harmony/typedar... > test/mjsunit/harmony/typedarrays-of.js:143: assertEquals(true, > constructor.of(val) instanceof constructor); > this doesn't appear to test what you intend to test -- you want > contstructor.of.call(val) i think Also, reading the spec more carefully, “%TypedArray%.of()” should throw in those cases (contrary to “Array.of()”, which uses “Array” as default constructor if the receiver is not a constructor/function). > https://codereview.chromium.org/660863003/diff/1/test/mjsunit/harmony/typedar... > test/mjsunit/harmony/typedarrays-of.js:147: for (var i = 0; i < > typedArrayConstructors.length; i++) { > for (var constructor of typedArrayConstructors) { > }
Could you please take a look after the patch set update from yesterday? Thanks!
LGTM with some nits in the tests; please add an OWNER :) https://codereview.chromium.org/660863003/diff/20001/test/mjsunit/harmony/typ... File test/mjsunit/harmony/typedarrays-of.js (right): https://codereview.chromium.org/660863003/diff/20001/test/mjsunit/harmony/typ... test/mjsunit/harmony/typedarrays-of.js:36: assertEquals(true, !!a[3]); just assert that a[3] is 1; more direct. likewhere with a[4] and 0, and below in the other case. https://codereview.chromium.org/660863003/diff/20001/test/mjsunit/harmony/typ... test/mjsunit/harmony/typedarrays-of.js:38: assertEquals(true, (a[5] - 3.14) < 1e-6); this needs abs()
aperez@igalia.com changed reviewers: + danno@chromium.org, dslomov@chromium.org
I have fixed the nits and added owners, PTAL.
lgtm
Message was sent while issue was closed.
Landed as ddcd08b1d1c07bf0a9c0d619068f0225972bf3c6. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
