Implement .fill() on typed arrays

Implement .fill() on typed arrays

BUG=v8:3578
LOG=Y

[aperez, 2014-11-04 21:14:37]

aperez@igalia.com changed reviewers:
+ wingo@igalia.com

[wingo, 2014-11-12 09:29:56]

https://codereview.chromium.org/699953003/diff/1/src/harmony-typedarray.js
File src/harmony-typedarray.js (right):

https://codereview.chromium.org/699953003/diff/1/src/harmony-typedarray.js#ne...
src/harmony-typedarray.js:67: var array = ToObject(this);
Need a check that the object is indeed a typed array.  See
http://people.mozilla.org/~jorendorff/es6-draft.html#sec-%typedarray%.prototy....
 No check needed that the array is of any particular kind, though.  (Didn't we
go through this on a different patch?  I don't recall.)

https://codereview.chromium.org/699953003/diff/1/src/harmony-typedarray.js#ne...
src/harmony-typedarray.js:68: var length = ToLength(array.length);
Need to get the array's "length" internal slot, not the length property.

https://codereview.chromium.org/699953003/diff/1/src/harmony-typedarray.js#ne...
src/harmony-typedarray.js:75: i = IS_UNDEFINED(i) ? 0 : TO_INTEGER(i);
I think that here we need i = TO_INTEGER(i) in all cases; "undefined" isn't
special.

https://codereview.chromium.org/699953003/diff/1/src/harmony-typedarray.js#ne...
src/harmony-typedarray.js:78: end = IS_UNDEFINED(end) ? length :
TO_INTEGER(end);
Very strangely, this logic is correct for "end" but not for "start".  Spec WTF?

https://codereview.chromium.org/699953003/diff/1/src/harmony-typedarray.js#ne...
src/harmony-typedarray.js:110: InstallFunctions(global.NAME.prototype,
DONT_ENUM, $Array(
Both of these should be DONT_WRITE and DONT_DELETE as well.

[aperez, 2014-11-13 16:34:36]

As an extra, I will be adding also code in the test
case to check that the method can be transplanted to
other typed array types, and to non-typed array
objects, as per the spec.

https://codereview.chromium.org/699953003/diff/1/src/harmony-typedarray.js
File src/harmony-typedarray.js (right):

https://codereview.chromium.org/699953003/diff/1/src/harmony-typedarray.js#ne...
src/harmony-typedarray.js:67: var array = ToObject(this);
On 2014/11/12 09:29:56, wingo wrote:
> Need a check that the object is indeed a typed array.  See
>
http://people.mozilla.org/~jorendorff/es6-draft.html#sec-%25typedarray%25.pro....
>  No check needed that the array is of any particular kind, though.  (Didn't we
> go through this on a different patch?  I don't recall.)

well, CHECK_OBJECT_COERCIBLE() just checks that the
arguments is not null/undefined, and that is not an
“undetectable object”, so this is not checking for a
typed array, just making sure the argument is something
we can operate on.

In the case of .fill() we do not have to check that the
receiver is a typed array. Or, well, the spec does not say
a thing about such check, and moreover it says at the bottom
of the section:

   “The fill function is intentionally generic; it does
   not require that its this value be an Array object.
   Therefore it can be transferred to other kinds of
   objects for use as a method.”

Therefore, I believe the code is correct as-is.

https://codereview.chromium.org/699953003/diff/1/src/harmony-typedarray.js#ne...
src/harmony-typedarray.js:68: var length = ToLength(array.length);
On 2014/11/12 09:29:56, wingo wrote:
> Need to get the array's "length" internal slot, not the length property.

The spec, in step 3 reads:

  3. Let lenVal be Get(O, "length").

…and Get() is specified to retrieve values of properties.
There is no reference to internal slots at all in the
definition of %TypedArray%.prototype.fill(), so I believe
the current code to be correct.

https://codereview.chromium.org/699953003/diff/1/src/harmony-typedarray.js#ne...
src/harmony-typedarray.js:75: i = IS_UNDEFINED(i) ? 0 : TO_INTEGER(i);
On 2014/11/12 09:29:56, wingo wrote:
> I think that here we need i = TO_INTEGER(i) in all cases; "undefined" isn't
> special.

Well, according to the spec, the first step of ToInteger()
is to call ToNumber(), which returns NaN for undefined, and
ToInteger() will then return 0 for NaN itself... So, the
bottom line is that there is no no need to check for
undefined because ToInteger(undefined) → 0.

I will be changing this to just: i = TO_INTEGER(i)

https://codereview.chromium.org/699953003/diff/1/src/harmony-typedarray.js#ne...
src/harmony-typedarray.js:78: end = IS_UNDEFINED(end) ? length :
TO_INTEGER(end);
On 2014/11/12 09:29:56, wingo wrote:
> Very strangely, this logic is correct for "end" but not for "start".  Spec
WTF?

For the case of end==undefined we want to take the length
of the array, and if ToInteger() would be used directly,
then we would get 0 for undefined (see previous comment).

It seems the spec is okay after all...

https://codereview.chromium.org/699953003/diff/1/src/harmony-typedarray.js#ne...
src/harmony-typedarray.js:110: InstallFunctions(global.NAME.prototype,
DONT_ENUM, $Array(
On 2014/11/12 09:29:56, wingo wrote:
> Both of these should be DONT_WRITE and DONT_DELETE as well.

Acknowledged.

[aperez, 2014-11-13 16:38:11]

Updated, I think we are good now to add owners to the review; PTAL.

[wingo, 2014-11-14 14:27:50]

https://codereview.chromium.org/699953003/diff/1/src/harmony-typedarray.js
File src/harmony-typedarray.js (right):

https://codereview.chromium.org/699953003/diff/1/src/harmony-typedarray.js#ne...
src/harmony-typedarray.js:67: var array = ToObject(this);
On 2014/11/13 16:34:36, aperez wrote:
> On 2014/11/12 09:29:56, wingo wrote:
> > Need a check that the object is indeed a typed array.  See
> >
>
http://people.mozilla.org/~jorendorff/es6-draft.html#sec-%25typedarray%25.pro....
> >  No check needed that the array is of any particular kind, though.  (Didn't
we
> > go through this on a different patch?  I don't recall.)
> 
> well, CHECK_OBJECT_COERCIBLE() just checks that the
> arguments is not null/undefined, and that is not an
> “undetectable object”, so this is not checking for a
> typed array, just making sure the argument is something
> we can operate on.
> 
> In the case of .fill() we do not have to check that the
> receiver is a typed array. Or, well, the spec does not say
> a thing about such check, and moreover it says at the bottom
> of the section:
> 
>    “The fill function is intentionally generic; it does
>    not require that its this value be an Array object.
>    Therefore it can be transferred to other kinds of
>    objects for use as a method.”
> 
> Therefore, I believe the code is correct as-is.

Please see
http://people.mozilla.org/~jorendorff/es6-draft.html#sec-%25typedarray%25.pro...,
which I pasted above.  The typed array fill function is not generic.

[aperez, 2014-11-14 15:27:23]

On 2014/11/14 14:27:50, wingo wrote:
> https://codereview.chromium.org/699953003/diff/1/src/harmony-typedarray.js
> File src/harmony-typedarray.js (right):
> 
>
https://codereview.chromium.org/699953003/diff/1/src/harmony-typedarray.js#ne...
> src/harmony-typedarray.js:67: var array = ToObject(this);
> On 2014/11/13 16:34:36, aperez wrote:
> > On 2014/11/12 09:29:56, wingo wrote:
> > > Need a check that the object is indeed a typed array.  See
> > >
> >
>
http://people.mozilla.org/~jorendorff/es6-draft.html#sec-%25typedarray%25.pro....
> > >  No check needed that the array is of any particular kind, though. 
(Didn't
> we
> > > go through this on a different patch?  I don't recall.)
> > 
> > well, CHECK_OBJECT_COERCIBLE() just checks that the
> > arguments is not null/undefined, and that is not an
> > “undetectable object”, so this is not checking for a
> > typed array, just making sure the argument is something
> > we can operate on.
> > 
> > In the case of .fill() we do not have to check that the
> > receiver is a typed array. Or, well, the spec does not say
> > a thing about such check, and moreover it says at the bottom
> > of the section:
> > 
> >    “The fill function is intentionally generic; it does
> >    not require that its this value be an Array object.
> >    Therefore it can be transferred to other kinds of
> >    objects for use as a method.”
> > 
> > Therefore, I believe the code is correct as-is.
> 
> Please see
>
http://people.mozilla.org/~jorendorff/es6-draft.html#sec-%25typedarray%25.pro...,
> which I pasted above.  The typed array fill function is not generic.

Right, I either was looking at the wrong section, or at an old version
of the spec... I will change this to work on typed arrays only and use
the internal slot. Sorry for the noise.

[aperez, 2014-11-14 15:28:32]

Updated, now we should be good to go for real :)