1121935 - Implement %TypedArray%.prototype.slice

Status: RESOLVED FIXED

(Core :: JavaScript: Standard Library, defect)

Comment 1

[Dirkjan Ochtman (:djc)]

Attachment: Part 1: Patch from :till for GetBuiltinConstructor (from bug 1121936)

Comment 2

[Dirkjan Ochtman (:djc)]

Attachment: Part 2: Implement SpeciesConstructor stub

Comment 3

[Dirkjan Ochtman (:djc)]

Attachment: Part 3: Initial implementation of %TypedArray%.slice()

Comment 4

[Dirkjan Ochtman (:djc)]

Attachment: Part 4: starting to implement some tests

Comment 5

[Dirkjan Ochtman (:djc)]

Spec, for reference: https://people.mozilla.org/~jorendorff/es6-draft.html#sec-%typedarray%.prototype.slice

Comment 6

[Dirkjan Ochtman (:djc)]

Attachment: Part 2: Implement SpeciesConstructor stub (fixed)

Comment 7

[Dirkjan Ochtman (:djc)]

Attachment: Part 4: Tests for %TypedArray%.slice()

Comment 8

[Dirkjan Ochtman (:djc)]

Attachment: MozReview Request: bz://1121935/djc

/r/3883 - Bug 1121935 - Part 1: add intrinsic for retrieving original builtin constructors. r=waldo
/r/3885 - Bug 1121935: Implement SpeciesConstructor stub for now
/r/3887 - Bug 1121935: Implement %TypedArray%.slice()
/r/3889 - Bug 1121935: Add tests for %TypedArray%.slice()

Pull down these commits:

hg pull review -r f1cb7cdc725e6475b01a1bc84487c198e09e6eed

Comment 9

[Tom S [:evilpie]]

Comment on attachment 8564485 [details]
MozReview Request: bz://1121935/djc

https://reviewboard.mozilla.org/r/3881/#review3079

::: js/src/builtin/TypedArray.js
(Diff revision 1)
> +function TypedArraySlice(start, end = undefined) {

end = undefined is wrong. The spec says: "The length property of the slice method is 2."

::: js/src/tests/ecma_6/TypedArray/slice.js
(Diff revision 1)
> +    assertDeepEq(constructor.prototype.slice.length, 1);

Would be 2 after the change above.

::: js/src/tests/ecma_6/TypedArray/slice.js
(Diff revision 1)
> +

I would like to see a few tests where start and/or end are outside of the bounds.

Like:
new constructor([1, 2]).slice(-3) // [1, 2]

new constructor([1, 2]).slice(0, -3) // [] ? I think

::: js/src/builtin/TypedArray.js
(Diff revision 1)
> +        // Steps 18a-e.

We usually do 18.a-e.

::: js/src/tests/ecma_6/TypedArray/slice.js
(Diff revision 1)
> +        }, TypeError, "Assert that reverse fails if this value is not a TypedArray");

Nit: s/reverse/slice/

::: js/src/tests/ecma_6/TypedArray/slice.js
(Diff revision 1)
> +

We should have a test where we test some of the SpeciesConstructor bits that we have already implemented.

var x = new constructor;
x.constructor = undefined;
slice.call({constructor: 123});

should work

x.constructor = "not a constructor";
.. slice ..
x.constructor = Math.sin; // also not a constructor
.. slice ..

should throw

Comment 10

[Tom S [:evilpie]]

Looks really good, I would just like to see tests for a few more edge cases.

Comment 11

[Dirkjan Ochtman (:djc)]

Comment on attachment 8564485 [details]
MozReview Request: bz://1121935/djc

/r/3883 - Bug 1121935 - Part 1: add intrinsic for retrieving original builtin constructors. r=waldo
/r/3885 - Bug 1121935: Implement SpeciesConstructor stub for now
/r/3887 - Bug 1121935: Implement %TypedArray%.slice()
/r/3889 - Bug 1121935: Add tests for %TypedArray%.slice()

Pull down these commits:

hg pull review -r 3e4ca896fc349bbef9a81bece1a89ff5cb3b935e

Comment 12

[Dirkjan Ochtman (:djc)]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ddebbe02dc59

Comment 13

[Tom S [:evilpie]]

Comment on attachment 8564485 [details]
MozReview Request: bz://1121935/djc

https://reviewboard.mozilla.org/r/3881/#review3087

Ship It!

::: js/src/tests/ecma_6/TypedArray/slice.js
(Diff revision 2)
> +    undefConstructor.slice(123);

It would be good to actually make sure something sensible happens. (You can fix this before landing, no new review required)

var undefConstructor = new constructor(2);
undefinedConstructor.constructor = undefined;
assertEq(undefConstructor.slice(1), new constructor(1))

Comment 14

[Tom S [:evilpie]]

I think you have to wait on bug 1121936, because that bug contains the patch for GetBuiltinConstructor, which is still waiting for review.

Comment 15

[Till Schneidereit [:till]]

Attachment: Part 1: add intrinsic for retrieving the original constructor for typed array instances. r=waldo

Moved over from bug 1121936.

Comment 16

[Tom S [:evilpie]]

Landed this! Thanks for all your effort, and I am sorry this was a little bit more difficult to get organized.
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5ec2e74a50c
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae1809cd39e3

Comment 17

[Dirkjan Ochtman (:djc)]

No worries, I think this was my smoothest patch cycle yet. Thanks for all the support!

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ae1809cd39e3
https://hg.mozilla.org/mozilla-central/rev/b5ec2e74a50c

Comment 19

[Florian Scholz (Open Web Docs)]

https://developer.mozilla.org/en-US/Firefox/Releases/38#JavaScript
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/TypedArray/slice

Comment 21

[Dirkjan Ochtman (:djc)]

Attachment: MozReview Request: Bug 1121935: Add tests for %TypedArray%.slice()

Comment 22

[Dirkjan Ochtman (:djc)]

Attachment: MozReview Request: Bug 1121935 - Part 1: add intrinsic for retrieving original builtin constructors. r=waldo

Comment 23

[Dirkjan Ochtman (:djc)]

Attachment: MozReview Request: Bug 1121935: Implement SpeciesConstructor stub for now

Comment 24

[Dirkjan Ochtman (:djc)]

Attachment: MozReview Request: Bug 1121935: Implement %TypedArray%.slice()