Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
376623004:
[es6] implement Array.prototype.copyWithin() (Closed)
|
Created:
6 years, 5 months ago by caitp (gmail) Modified:
5 years, 8 months ago CC:
v8-dev Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[es6] implement Array.prototype.copyWithin()
https://people.mozilla.org/~jorendorff/es6-draft.html#sec-array.prototype.copywithin
BUG=v8:4039
R=adamk@chromium.org
LOG=N
Committed: https://crrev.com/6d00703e5efacd5677b44067d46d687b6f4877ca
Cr-Commit-Position: refs/heads/master@{#27983}
Patch Set 1 #
Total comments: 24
Patch Set 2 : rebase #
Total comments: 1
Patch Set 3 : adjustments, test improvements #Patch Set 4 : the number is implicitly ToObject()'d anyways #Patch Set 5 : fix expectations #
Total comments: 4
Patch Set 6 : Re-add CHECK_OBJECT_COERCIBLE, nits, extra large object test #Patch Set 7 : rebase #Messages
Total messages: 19 (5 generated)
Since we've got Array.from and Array.of CLs open, why not one for Array.prototype.copyWithin as well? Most of these tests have been lifted from the spidermonkey test suite and refactored to fit mjsunit.
caitpotter88@gmail.com changed reviewers: + dslomov@chromium.org
caitpotter88@gmail.com changed reviewers: + rossberg@chromium.org
PTAL --- it would be nice to get some comments on this one =)
adamk@chromium.org changed reviewers: + adamk@chromium.org
Looking good, all non-test comments are stylistic. The test looks very thorough too, just a few rough edges. https://codereview.chromium.org/376623004/diff/1/src/harmony-array.js File src/harmony-array.js (right): https://codereview.chromium.org/376623004/diff/1/src/harmony-array.js#newcode14 src/harmony-array.js:14: function ArrayCopyWithin(target, start /* [ , end ] */ ) { // length == 2 Another option here is to name the argument and use %FunctionSetLength down below to get the length right. Seems like that's more common elsewhere, while this file prefers this style. https://codereview.chromium.org/376623004/diff/1/src/harmony-array.js#newcode17 src/harmony-array.js:17: var array = ToObject(this); TO_OBJECT_INLINE https://codereview.chromium.org/376623004/diff/1/src/harmony-array.js#newcode18 src/harmony-array.js:18: var length = ToInteger(array.length); TO_UINT32 (hmm, we should figure out this ToLength stuff at some point). https://codereview.chromium.org/376623004/diff/1/src/harmony-array.js#newcode21 src/harmony-array.js:21: var relativeEnd = length; I'd move this down to its initialization to make it clear that it gets the right value. I'd prefer the rest move down, too, to just before their initialization. https://codereview.chromium.org/376623004/diff/1/src/harmony-array.js#newcode25 src/harmony-array.js:25: var direction = 1; Same here, moving this down will make it clearer I think. https://codereview.chromium.org/376623004/diff/1/src/harmony-array.js#newcode28 src/harmony-array.js:28: target = ToInteger(target); TO_INTEGER (and similar below) https://codereview.chromium.org/376623004/diff/1/src/harmony-array.js#newcode30 src/harmony-array.js:30: to = MathMax(length + target, 0); I think this is now spelled $max() https://codereview.chromium.org/376623004/diff/1/src/harmony-array.js#newcode32 src/harmony-array.js:32: to = MathMin(target, length); and this is $min() https://codereview.chromium.org/376623004/diff/1/test/mjsunit/harmony/array-c... File test/mjsunit/harmony/array-copywithin.js (right): https://codereview.chromium.org/376623004/diff/1/test/mjsunit/harmony/array-c... test/mjsunit/harmony/array-copywithin.js:41: // assertThrows('Array.prototype.copyWithin.call("hello world", 0, 3);', TypeError); This throws because the indices are not writable. You should be able to construct a non-throwing case. https://codereview.chromium.org/376623004/diff/1/test/mjsunit/harmony/array-c... test/mjsunit/harmony/array-copywithin.js:100: // check that delete is strict Also want to check that Set() is strict (your original String case fails for that reason, I suppose you could re-use it for this case but it's a little obscure). https://codereview.chromium.org/376623004/diff/1/test/mjsunit/harmony/array-c... test/mjsunit/harmony/array-copywithin.js:112: // FIXME: Support Proxies You can probably test this with our existing implementation, adding --harmony-proxies. https://codereview.chromium.org/376623004/diff/1/test/mjsunit/harmony/array-c... test/mjsunit/harmony/array-copywithin.js:171: assertArrayEquals([4, 5, 3, 4, 5], [1, 2, 3, 4, 5].copyWithin(0, 3)); I think this test is probably more interesting if you have a hole in this array.
https://codereview.chromium.org/376623004/diff/1/test/mjsunit/harmony/array-c... File test/mjsunit/harmony/array-copywithin.js (right): https://codereview.chromium.org/376623004/diff/1/test/mjsunit/harmony/array-c... test/mjsunit/harmony/array-copywithin.js:41: // assertThrows('Array.prototype.copyWithin.call("hello world", 0, 3);', TypeError); On 2015/04/17 22:34:17, adamk wrote: > This throws because the indices are not writable. You should be able to > construct a non-throwing case. I think these tests are all wrong, since a target is not being passed in, I'm gonna overhaul these. good catch (it's the "0" whose index is non-writable, though, not the "hello world")
On 2015/04/17 22:36:50, caitp wrote: > https://codereview.chromium.org/376623004/diff/1/test/mjsunit/harmony/array-c... > File test/mjsunit/harmony/array-copywithin.js (right): > > https://codereview.chromium.org/376623004/diff/1/test/mjsunit/harmony/array-c... > test/mjsunit/harmony/array-copywithin.js:41: // > assertThrows('Array.prototype.copyWithin.call("hello world", 0, 3);', > TypeError); > On 2015/04/17 22:34:17, adamk wrote: > > This throws because the indices are not writable. You should be able to > > construct a non-throwing case. > > I think these tests are all wrong, since a target is not being passed in, I'm > gonna overhaul these. good catch (it's the "0" whose index is non-writable, > though, not the "hello world") can I delete that last comment I wrote somehow? it was totally wrong
https://codereview.chromium.org/376623004/diff/1/src/harmony-array.js File src/harmony-array.js (right): https://codereview.chromium.org/376623004/diff/1/src/harmony-array.js#newcode14 src/harmony-array.js:14: function ArrayCopyWithin(target, start /* [ , end ] */ ) { // length == 2 On 2015/04/17 22:34:17, adamk wrote: > Another option here is to name the argument and use %FunctionSetLength down > below to get the length right. Seems like that's more common elsewhere, while > this file prefers this style. Done, I prefer the %FunctionSetLength() approach https://codereview.chromium.org/376623004/diff/1/src/harmony-array.js#newcode17 src/harmony-array.js:17: var array = ToObject(this); On 2015/04/17 22:34:16, adamk wrote: > TO_OBJECT_INLINE Done. https://codereview.chromium.org/376623004/diff/1/src/harmony-array.js#newcode18 src/harmony-array.js:18: var length = ToInteger(array.length); On 2015/04/17 22:34:17, adamk wrote: > TO_UINT32 (hmm, we should figure out this ToLength stuff at some point). We've had ToLength() for a while --- not sure how inline-able it is, but it should be doable. Went with that https://codereview.chromium.org/376623004/diff/1/src/harmony-array.js#newcode21 src/harmony-array.js:21: var relativeEnd = length; On 2015/04/17 22:34:16, adamk wrote: > I'd move this down to its initialization to make it clear that it gets the right > value. I'd prefer the rest move down, too, to just before their initialization. Done. https://codereview.chromium.org/376623004/diff/1/src/harmony-array.js#newcode25 src/harmony-array.js:25: var direction = 1; On 2015/04/17 22:34:16, adamk wrote: > Same here, moving this down will make it clearer I think. Done. https://codereview.chromium.org/376623004/diff/1/src/harmony-array.js#newcode28 src/harmony-array.js:28: target = ToInteger(target); On 2015/04/17 22:34:17, adamk wrote: > TO_INTEGER (and similar below) Done. https://codereview.chromium.org/376623004/diff/1/src/harmony-array.js#newcode30 src/harmony-array.js:30: to = MathMax(length + target, 0); On 2015/04/17 22:34:17, adamk wrote: > I think this is now spelled $max() Done. https://codereview.chromium.org/376623004/diff/1/src/harmony-array.js#newcode32 src/harmony-array.js:32: to = MathMin(target, length); On 2015/04/17 22:34:17, adamk wrote: > and this is $min() Done. https://codereview.chromium.org/376623004/diff/1/test/mjsunit/harmony/array-c... File test/mjsunit/harmony/array-copywithin.js (right): https://codereview.chromium.org/376623004/diff/1/test/mjsunit/harmony/array-c... test/mjsunit/harmony/array-copywithin.js:41: // assertThrows('Array.prototype.copyWithin.call("hello world", 0, 3);', TypeError); On 2015/04/17 22:36:50, caitp wrote: > On 2015/04/17 22:34:17, adamk wrote: > > This throws because the indices are not writable. You should be able to > > construct a non-throwing case. > > I think these tests are all wrong, since a target is not being passed in, I'm > gonna overhaul these. good catch (it's the "0" whose index is non-writable, > though, not the "hello world") I apparently forgot how copyWithin() worked when I wrote that comment --- so this should throw, but we have a bug which seemed to not be filed, so I filed it https://code.google.com/p/v8/issues/detail?id=4042 https://codereview.chromium.org/376623004/diff/1/test/mjsunit/harmony/array-c... test/mjsunit/harmony/array-copywithin.js:100: // check that delete is strict On 2015/04/17 22:34:17, adamk wrote: > Also want to check that Set() is strict (your original String case fails for > that reason, I suppose you could re-use it for this case but it's a little > obscure). Done, with some various [[Extensible]]-related tests too https://codereview.chromium.org/376623004/diff/1/test/mjsunit/harmony/array-c... test/mjsunit/harmony/array-copywithin.js:112: // FIXME: Support Proxies On 2015/04/17 22:34:17, adamk wrote: > You can probably test this with our existing implementation, adding > --harmony-proxies. IMO, better to just get rid of the test for now. eventually it will be necessary to add proxy tests for all the standard lib stuff, if they are ever implemented properly.
Looking good, just a few nits and one test request. https://codereview.chromium.org/376623004/diff/20001/src/harmony-array.js File src/harmony-array.js (right): https://codereview.chromium.org/376623004/diff/20001/src/harmony-array.js#new... src/harmony-array.js:15: CHECK_OBJECT_COERCIBLE(this, "Array.prototype.copyWithin"); For consistency I feel like we might as well leave this here (otherwise this method gives a different error message than every other method). It's possible we should do some cleanup in future, though, to get rid of all of these together (see https://code.google.com/p/v8/issues/detail?id=3577). https://codereview.chromium.org/376623004/diff/80001/src/harmony-array.js File src/harmony-array.js (right): https://codereview.chromium.org/376623004/diff/80001/src/harmony-array.js#new... src/harmony-array.js:16: var length = ToLength(array.length); Please add a test with an object whose length > 2^32-1 (can't be an Array, obviously). https://codereview.chromium.org/376623004/diff/80001/src/harmony-array.js#new... src/harmony-array.js:48: var direction = 1; One more style nit: move this to just before the if statement, and remove "var" from the "direction = -1" inside the if, above.
https://codereview.chromium.org/376623004/diff/80001/src/harmony-array.js File src/harmony-array.js (right): https://codereview.chromium.org/376623004/diff/80001/src/harmony-array.js#new... src/harmony-array.js:16: var length = ToLength(array.length); On 2015/04/20 17:26:14, adamk wrote: > Please add a test with an object whose length > 2^32-1 (can't be an Array, > obviously). Done. https://codereview.chromium.org/376623004/diff/80001/src/harmony-array.js#new... src/harmony-array.js:48: var direction = 1; On 2015/04/20 17:26:14, adamk wrote: > One more style nit: move this to just before the if statement, and remove "var" > from the "direction = -1" inside the if, above. Done.
lgtm Thanks for the ping on this one, happy to see harmony-array complete (modulo typed arrays stuff).
On 2015/04/20 17:57:48, adamk wrote: > lgtm > > Thanks for the ping on this one, happy to see harmony-array complete (modulo > typed arrays stuff). Thanks for the review =) dslomov@ any other comments before landing?
The CQ bit was checked by caitpotter88@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from adamk@chromium.org Link to the patchset: https://codereview.chromium.org/376623004/#ps120001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/376623004/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:120001) has been created in https://codereview.chromium.org/1084183004/ by machenbach@chromium.org. The reason for reverting is: [Sheriff] This causes test failures on mac gc stress: http://build.chromium.org/p/client.v8/builders/V8%20Mac%20GC%20Stress/builds/....
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/6d00703e5efacd5677b44067d46d687b6f4877ca Cr-Commit-Position: refs/heads/master@{#27983} |
