Array.prototype.{reduce, reduceRight}: Wrong order of operations when determining initial value.

Array.prototype.{reduce, reduceRight}: Wrong order of operations when determining initial value.

BUG=v8:3534
LOG=

[Diego Pino, 2014-09-29 17:27:24]

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

[Diego Pino, 2014-10-03 19:09:21]

Please take a look when you have time :)

[wingo, 2014-10-07 11:04:32]

https://codereview.chromium.org/614733002/diff/1/src/array.js
File src/array.js (right):

https://codereview.chromium.org/614733002/diff/1/src/array.js#newcode1399
src/array.js:1399: }
I think the test should be %_ArgumentsLength() < 2 instead of
IS_UNDEFINED(current); explicitly passing undefined is fine.

https://codereview.chromium.org/614733002/diff/1/src/array.js#newcode1438
src/array.js:1438: }
likewise

https://codereview.chromium.org/614733002/diff/1/test/mjsunit/array-reduce.js
File test/mjsunit/array-reduce.js (right):

https://codereview.chromium.org/614733002/diff/1/test/mjsunit/array-reduce.js...
test/mjsunit/array-reduce.js:532: Object.defineProperty(a, "0", {get: function()
{ delete this[0] },
Inconsistent spacing here (before the get), and i find the two-line break for
this descriptor a bit wonky; maybe that's just me.

The test is better as:
assertEquals(undefined, a.reduce(function (a) { return a }))

[arv (Not doing code reviews), 2014-10-07 14:16:02]

arv@chromium.org changed reviewers:
+ arv@chromium.org

[arv (Not doing code reviews), 2014-10-07 14:16:03]

https://codereview.chromium.org/614733002/diff/1/src/array.js
File src/array.js (right):

https://codereview.chromium.org/614733002/diff/1/src/array.js#newcode1399
src/array.js:1399: }
On 2014/10/07 11:04:31, wingo wrote:
> I think the test should be %_ArgumentsLength() < 2 instead of
> IS_UNDEFINED(current); explicitly passing undefined is fine.

Yup. The spec says: "If initialValue is present, then" so an args length check
is needed.

https://people.mozilla.org/~jorendorff/es6-draft.html#sec-array.prototype.reduce

https://codereview.chromium.org/614733002/diff/1/src/array.js#newcode1438
src/array.js:1438: }
On 2014/10/07 11:04:31, wingo wrote:
> likewise

Yup.
https://people.mozilla.org/~jorendorff/es6-draft.html#sec-array.prototype.red...

[Diego Pino, 2014-10-09 10:18:57]

https://codereview.chromium.org/614733002/diff/1/src/array.js
File src/array.js (right):

https://codereview.chromium.org/614733002/diff/1/src/array.js#newcode1399
src/array.js:1399: }
On 2014/10/07 11:04:31, wingo wrote:
> I think the test should be %_ArgumentsLength() < 2 instead of
> IS_UNDEFINED(current); explicitly passing undefined is fine.

What I intended to code with that line was step 7 of the algorithm.

7. If len is 0 and initialValue is not present, throw a TypeError exception.
(https://people.mozilla.org/~jorendorff/es6-draft.html#sec-array.prototype.reduce).

However I realized is not needed actually, because the code below that searches
for initialValue, when is not defined (%_ArgumentsLength < 2), covers that case
too. There's already a test for this (array length equals 0 and current equals
undefined).

[Diego Pino, 2014-10-09 10:21:14]

Uploaded a new patch fixing last issues. Please, take a look. Thanks :)

[wingo, 2014-10-13 12:37:02]

lgtm

[Diego Pino, 2014-10-16 16:37:26]

dpino@igalia.com changed reviewers:
+ svenpanne@chromium.org

[Diego Pino, 2014-10-16 16:40:32]

Sven Panne: "git cl owner" suggested you as an owner. The issue is already
reviewed by non-owners. Could you please take a look? Thanks :)

[Sven Panne, 2014-10-21 08:54:11]

lgtm

[wingo (chromium), 2014-10-21 10:02:46]

i'll land

[wingo (chromium), 2014-10-22 13:15:24]

Committed patchset #2 manually as 24807 (presubmit successful).