Implement %TypedArray%.prototype.slice

RESOLVED FIXED in Firefox 38

Status

()

Core
JavaScript: Standard Library
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: ziyunfei, Assigned: djc)

Tracking

({dev-doc-complete})

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

Firefox Tracking Flags

(firefox38 fixed)

Details

(Whiteboard: [DocArea=JS])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

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 | Review
39 bytes, text/x-review-board-request
evilpie
: review+
Details | Review
39 bytes, text/x-review-board-request
evilpie
: review+
Details | Review
39 bytes, text/x-review-board-request
evilpie
: review+
Details | Review
Comment hidden (empty)
(Assignee)

Comment 1

3 years ago
Created attachment 8561074 [details] [diff] [review]
Part 1: Patch from :till for GetBuiltinConstructor (from bug 1121936)
Assignee: 446240525 → dirkjan
Status: NEW → ASSIGNED
(Assignee)

Comment 2

3 years ago
Created attachment 8561075 [details] [diff] [review]
Part 2: Implement SpeciesConstructor stub
(Assignee)

Comment 3

3 years ago
Created attachment 8561076 [details] [diff] [review]
Part 3: Initial implementation of %TypedArray%.slice()
(Assignee)

Comment 4

3 years ago
Created attachment 8561078 [details] [diff] [review]
Part 4: starting to implement some tests
(Assignee)

Updated

3 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 5

3 years ago
Spec, for reference: https://people.mozilla.org/~jorendorff/es6-draft.html#sec-%typedarray%.prototype.slice
(Assignee)

Comment 6

3 years ago
Created attachment 8561096 [details] [diff] [review]
Part 2: Implement SpeciesConstructor stub (fixed)
Attachment #8561075 - Attachment is obsolete: true
(Assignee)

Comment 7

3 years ago
Created attachment 8561097 [details] [diff] [review]
Part 4: Tests for %TypedArray%.slice()
Attachment #8561078 - Attachment is obsolete: true
(Assignee)

Comment 8

3 years ago
Created 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 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

3 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)
(Assignee)

Comment 12

3 years ago
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ddebbe02dc59
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.
Created attachment 8566168 [details] [diff] [review]
Part 1: add intrinsic for retrieving the original constructor for typed array instances. r=waldo

Moved over from bug 1121936.
Attachment #8566168 - Flags: review+
Blocks: 1121936
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

3 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: 3 years ago
status-firefox38: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
https://developer.mozilla.org/en-US/Firefox/Releases/38#JavaScript
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/TypedArray/slice
Keywords: dev-doc-needed → dev-doc-complete

Updated

3 years ago
Depends on: 1167700
(Assignee)

Comment 20

3 years ago
Comment on attachment 8564485 [details]
MozReview Request: bz://1121935/djc
Attachment #8564485 - Attachment is obsolete: true
Attachment #8619135 - Flags: review+
Attachment #8619136 - Flags: review+
Attachment #8619137 - Flags: review+
Attachment #8619138 - Flags: review+
(Assignee)

Comment 21

3 years ago
Created attachment 8619135 [details]
MozReview Request: Bug 1121935: Add tests for %TypedArray%.slice()
(Assignee)

Comment 22

3 years ago
Created attachment 8619136 [details]
MozReview Request: Bug 1121935 - Part 1: add intrinsic for retrieving original builtin constructors. r=waldo
(Assignee)

Comment 23

3 years ago
Created attachment 8619137 [details]
MozReview Request: Bug 1121935: Implement SpeciesConstructor stub for now
(Assignee)

Comment 24

3 years ago
Created attachment 8619138 [details]
MozReview Request: Bug 1121935: Implement %TypedArray%.slice()

Updated

2 years ago
See Also: → bug 1232966
You need to log in before you can comment on or make changes to this bug.