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#newcode66
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#newcode78
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#newcode95
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/typedarrays-of.js
File test/mjsunit/harmony/typedarrays-of.js (right):
https://codereview.chromium.org/660863003/diff/1/test/mjsunit/harmony/typedarrays-of.js#newcode24
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/typedarrays-of.js#newcode33
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/typedarrays-of.js#newcode86
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/typedarrays-of.js#newcode87
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/typedarrays-of.js#newcode90
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/typedarrays-of.js#newcode135
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/typedarrays-of.js#newcode137
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/typedarrays-of.js#newcode142
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/typedarrays-of.js#newcode143
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/typedarrays-of.js#newcode147
test/mjsunit/harmony/typedarrays-of.js:147: for (var i = 0; i <
typedArrayConstructors.length; i++) {
for (var constructor of typedArrayConstructors) {
}
https://codereview.chromium.org/660863003/
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/d/optout.
