Implement %TypedArray%.prototype.slice

RESOLVED FIXED in Firefox 38

Status

()

defect
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: 446240525, Assigned: djc)

Tracking

({dev-doc-complete})

Trunk
mozilla38
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox38 fixed)

Details

(Whiteboard: [DocArea=JS])

Attachments

(5 attachments, 7 obsolete attachments)

3.00 KB, patch
till
: review+
Details | Diff | Splinter Review
39 bytes, text/x-review-board-request
evilpie
: review+
Details
39 bytes, text/x-review-board-request
evilpie
: review+
Details
39 bytes, text/x-review-board-request
evilpie
: review+
Details
39 bytes, text/x-review-board-request
evilpie
: review+
Details
Comment hidden (empty)
(Assignee)

Comment 1

4 years ago
Assignee: 446240525 → dirkjan
Status: NEW → ASSIGNED
(Assignee)

Updated

4 years ago
Attachment #8561074 - Attachment description: Patch from :till for GetBuiltinConstructor (from bug 1121936) → Part 1: Patch from :till for GetBuiltinConstructor (from bug 1121936)
(Assignee)

Comment 6

4 years ago
Attachment #8561075 - Attachment is obsolete: true
(Assignee)

Comment 7

4 years ago
Attachment #8561078 - Attachment is obsolete: true
(Assignee)

Comment 8

4 years ago
Posted file MozReview Request: bz://1121935/djc (obsolete) —
/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
Attachment #8564485 - Flags: review?(evilpies)
Attachment #8561074 - Attachment is obsolete: true
Attachment #8561076 - Attachment is obsolete: true
Attachment #8561096 - Attachment is obsolete: true
Attachment #8561097 - Attachment is obsolete: true
Attachment #8564485 - Flags: review?(evilpies)
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
Looks really good, I would just like to see tests for a few more edge cases.
(Assignee)

Comment 11

4 years ago
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
Attachment #8564485 - Flags: review?(evilpies)
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))
Attachment #8564485 - Flags: review?(evilpies) → review+
I think you have to wait on bug 1121936, because that bug contains the patch for GetBuiltinConstructor, which is still waiting for review.
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
(Assignee)

Comment 17

4 years ago
No worries, I think this was my smoothest patch cycle yet. Thanks for all the support!
https://hg.mozilla.org/mozilla-central/rev/ae1809cd39e3
https://hg.mozilla.org/mozilla-central/rev/b5ec2e74a50c
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla38

Updated

4 years ago
Depends on: 1167700
(Assignee)

Comment 20

4 years ago
Attachment #8564485 - Attachment is obsolete: true
Attachment #8619135 - Flags: review+
Attachment #8619136 - Flags: review+
Attachment #8619137 - Flags: review+
Attachment #8619138 - Flags: review+
See Also: → 1232966
You need to log in before you can comment on or make changes to this bug.