Brandon Benvie [:benvie] Reporter
	Description • 11 years ago

Two new Array and TypedArray methods have been added in the latest ES6 spec draft (August 2013 revision).

* Array.prototype.fill(value, start = 0, end = this.length) - 15.4.3.29
  Fill all the indices of an array between `start` and `end` with `value. Returns `ToObject(this)`.

* Array.prototype.copyWithin(target, start, end = this.length) - 15.4.3.30
  This method copies values from one part of an array to another. `target` is where to start copying to. `start` is where to start copying from. `end` is where copying ends at.

Nearly identical methods are to be added to TypedArrays, with the spec noting: "%TypedArray%.prototype.fill is a distinct function that implements the same algorithm as Array.prototype.fill as defined in 15.4.3.23. However, the implementation of the algorithm may be optimized to assume that the this value is an object that has a fixed length and whose integer indexed properties are not sparse.". See 15.13.6.3.26 and 15.13.6.3.27.

	Till Schneidereit [:till] Assignee
	Comment 1 • 11 years ago

Straight-forward self-hosted implementation. I hope I've covered all corner cases with the tests.

	Brandon Benvie [:benvie] Reporter
	Comment 2 • 10 years ago

Comment on attachment 8351463 [details] [diff] [review]
Part 1: Implement Array.prototype.fill

Review of attachment 8351463 [details] [diff] [review]:
-----------------------------------------------------------------

Looks mostly good.

::: js/src/builtin/Array.js
@@ +494,5 @@
> +    // Steps 1-2.
> +    var O = ToObject(this);
> +
> +    // Steps 3-5.
> +    var len = ToInteger(O.length);

Should probably add a FIXME pointing to bug 924058 ("Array operations should use ToLength").

	Jason Orendorff [:jorendorff]
	Comment 3 • 10 years ago

Note bug 934423. There is a massive comment in there by me listing things that could be tested... I'm afraid I may have killed the patch with that.

But we do want fairly thorough test coverage for this, because we want to optimize it.

	Till Schneidereit [:till] Assignee
	Comment 4 • 10 years ago

Oh, I didn't see bug 934423. Luckily, that deals with the method I didn't work on. :)

Jason, I'll look at which of the tests you propose apply to .fill and add them.

	Jason Orendorff [:jorendorff]
	Comment 5 • 10 years ago

Comment on attachment 8351463 [details] [diff] [review]
Part 1: Implement Array.prototype.fill

Review of attachment 8351463 [details] [diff] [review]:
-----------------------------------------------------------------

Oh no, I had completely forgotten about this. Please don't let me leave reviews idle this long!

::: js/src/builtin/Array.js
@@ +489,5 @@
>      return -1;
>  }
>  
> +// ES6 draft 2013-11-08 22.1.3.6
> +function ArrayFill(value/*, start = 0, end = this.length*/) {

I think you can actually say `value, start = 0, end = undefined` here now, since the .length of the resulting function will be 1.

@@ +499,5 @@
> +
> +    // Steps 6-7.
> +    var relativeStart = arguments.length > 1 && arguments[1] !== undefined
> +                        ? ToInteger(arguments[1])
> +                        : 0;

var relativeStart = ToInteger(start);

@@ +509,5 @@
> +
> +    // Steps 9-10.
> +    var relativeEnd = arguments.length > 2 && arguments[2] !== undefined
> +                      ? ToInteger(arguments[2])
> +                      : len;

var relativeEnd = end === undefined ? len : ToInteger(end);

@@ +518,5 @@
> +                : std_Math_min(relativeEnd, len);
> +
> +    // Step 12.
> +    for (; k < final; k++) {
> +        O[k] = value;

Should be a strict-mode assignment (and this needs a test).

Is it possible for self-hosting code to do stuff like this?

    function strictAssign(O, p, v) {
        "use strict";
        O[p] = v;
    }

I ask in part because the Array.from implementation in bug 904723 uses a trick like that; if it doesn't work, please comment there too!

::: js/src/tests/ecma_6/Array/fill.js
@@ +29,5 @@
> +assertDeepEq([1,1,1].fill(2, undefined, 1), [2,1,1]);
> +assertDeepEq([1,1,1].fill(2, 2, 1), [1,1,1]);
> +assertDeepEq([1,1,1].fill(2, -1, 1), [1,1,1]);
> +assertDeepEq([1,1,1].fill(2, -2, 1), [1,1,1]);
> +assertDeepEq([1,1,1].fill(2, 1, -2), [1,1,1]);

Test these stupid cases:
- an argument is a non-integer number, -0, NaN, infinity, a string, an object with a .valueOf method
  (It's ugly that the spec for ToInteger says it can return infinity. That's not an integer. Neither is -0. Grr.)
- this is a string primitive
- this is null or undefined
- this is a frozen Array
- this is a Proxy with a suitable "set" handler
- this is a typed array and the value to be assigned is an object with a .valueOf method (it should be called multiple times).
- this is a Uint8ClampedArray and the value is out of range.

	Till Schneidereit [:till] Assignee
	Comment 6 • 10 years ago

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f26809e69199

I added lots of tests, including the ones you requested.

	Ryan VanderMeulen [:RyanVM]
	Comment 7 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/f26809e69199

	u279076
	Comment 8 • 10 years ago

Assuming this needs to QA testing due to the extensive automated tests which landed. Please needinfo me if this needs further testing before we release Firefox 31.

	u279076
	Comment 9 • 10 years ago

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #8)
> Assuming this needs to QA testing due to the extensive automated tests which
> landed. Please needinfo me if this needs further testing before we release
> Firefox 31.

Sorry, that should read "Assuming this need no QA testing..."

	ziyunfei
	Comment 10 • 10 years ago

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/Array.prototype.fill()

	ziyunfei
	Comment 11 • 10 years ago

Moved to https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/fill

	Till Schneidereit [:till] Assignee
	Comment 12 • 10 years ago

(In reply to 446240525 from comment #11)
> Moved to
> https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/
> Global_Objects/Array/fill

Very nice! Especially the polyfill, which I tweeted about here:
https://twitter.com/tschneidereit/status/455027227949039616

	Florian Scholz (Open Web Docs)
	Comment 13 • 10 years ago

Thanks for documenting this!

I've also added it to "Firefox 31 for developers":
https://developer.mozilla.org/en-US/Firefox/Releases/31#JavaScript

	Chris Peterson [:cpeterson]
	Comment 14 • 10 years ago

ES6's Array.prototype.fill is probably worth relnoting.

	Sylvestre Ledru [:Sylvestre]
	Comment 15 • 10 years ago

Thanks. Added in the release notes for 31.