Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(29)

Issue 342453002: Arguments object has @@iterator (Closed)

Created:
5 years, 6 months ago by wingo
Modified:
5 years, 3 months ago
CC:
arv (Not doing code reviews), dcarney, Michael Starzinger, v8-dev, rossberg
Visibility:
Public.

Description

Arguments object has @@iterator R=arv@chromium.org, verwaest@chromium.org, rossberg@chromium.org BUG=v8:3391 LOG=N TEST=mjsunit/harmony/arguments-iterator.js Committed: https://code.google.com/p/v8/source/detail?r=23341

Patch Set 1 #

Total comments: 3

Patch Set 2 : Rebased, and updated to use API callbacks #

Total comments: 1

Patch Set 3 : Added more thorough test for own property descriptor #

Total comments: 5

Patch Set 4 : Fix accessor info for @@iterator, git cl format #

Patch Set 5 : Fix arguments object as prototype #

Total comments: 2

Patch Set 6 : Update to use SetPropertyOnInstanceIfInherited #

Patch Set 7 : rebased, adapted to getdataproperty change #

Unified diffs Side-by-side diffs Delta from patch set Stats (+331 lines, -22 lines) Patch
M src/accessors.h View 1 2 3 1 chunk +22 lines, -21 lines 0 comments Download
M src/accessors.cc View 1 2 3 4 5 6 1 chunk +40 lines, -0 lines 0 comments Download
M src/bootstrapper.cc View 1 2 3 4 5 6 4 chunks +31 lines, -0 lines 0 comments Download
M src/contexts.h View 1 2 chunks +3 lines, -1 line 0 comments Download
M src/runtime.cc View 1 2 3 4 5 6 1 chunk +4 lines, -0 lines 0 comments Download
A test/mjsunit/es6/arguments-iterator.js View 1 2 3 4 5 1 chunk +230 lines, -0 lines 0 comments Download
M test/mjsunit/mjsunit.status View 1 2 3 4 5 6 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 18 (0 generated)
wingo
PTAL. This deviates from the spec in two ways: 1) The @@iterator property appears after ...
5 years, 6 months ago (2014-06-17 11:31:55 UTC) #1
wingo
Ping :)
5 years, 5 months ago (2014-06-25 07:23:17 UTC) #2
arv (Not doing code reviews)
FYI. https://codereview.chromium.org/342453002/diff/1/src/array-iterator.js File src/array-iterator.js (right): https://codereview.chromium.org/342453002/diff/1/src/array-iterator.js#newcode152 src/array-iterator.js:152: get: ArgumentsIteratorGetter, It is not clear to me ...
5 years, 5 months ago (2014-06-25 15:55:05 UTC) #3
wingo
On 2014/06/25 15:55:05, arv wrote: > src/array-iterator.js:152: get: ArgumentsIteratorGetter, > It is not clear to ...
5 years, 5 months ago (2014-06-25 16:16:51 UTC) #4
rossberg
I agree that implementing it as a data property would be rather painful. I think ...
5 years, 5 months ago (2014-06-26 15:51:53 UTC) #5
wingo
Updated to add requested tests, and to make @@iterator a data property using API accessors ...
5 years, 4 months ago (2014-08-12 16:11:37 UTC) #6
arv (Not doing code reviews)
Andreas is out this week. Toon, can you take a look? LGTM except for the ...
5 years, 4 months ago (2014-08-12 16:31:53 UTC) #7
Toon Verwaest
https://codereview.chromium.org/342453002/diff/40001/src/accessors.cc File src/accessors.cc (right): https://codereview.chromium.org/342453002/diff/40001/src/accessors.cc#newcode184 src/accessors.cc:184: MaybeHandle<Object> maybe = Object::SetDataProperty(&it, value); This doesn't seem right ...
5 years, 3 months ago (2014-08-18 13:47:25 UTC) #8
arv (Not doing code reviews)
https://codereview.chromium.org/342453002/diff/40001/src/accessors.cc File src/accessors.cc (right): https://codereview.chromium.org/342453002/diff/40001/src/accessors.cc#newcode184 src/accessors.cc:184: MaybeHandle<Object> maybe = Object::SetDataProperty(&it, value); On 2014/08/18 13:47:25, Toon ...
5 years, 3 months ago (2014-08-18 14:16:56 UTC) #9
wingo
https://codereview.chromium.org/342453002/diff/40001/src/accessors.cc File src/accessors.cc (right): https://codereview.chromium.org/342453002/diff/40001/src/accessors.cc#newcode184 src/accessors.cc:184: MaybeHandle<Object> maybe = Object::SetDataProperty(&it, value); On 2014/08/18 13:47:25, Toon ...
5 years, 3 months ago (2014-08-20 15:46:57 UTC) #10
wingo
Um, On 2014/08/20 15:46:57, wingo wrote: > It has to be implemented using API accessor ...
5 years, 3 months ago (2014-08-20 15:55:39 UTC) #11
Toon Verwaest
https://codereview.chromium.org/342453002/diff/40001/src/accessors.cc File src/accessors.cc (right): https://codereview.chromium.org/342453002/diff/40001/src/accessors.cc#newcode184 src/accessors.cc:184: MaybeHandle<Object> maybe = Object::SetDataProperty(&it, value); To be clear what ...
5 years, 3 months ago (2014-08-21 11:07:20 UTC) #12
wingo
On 2014/08/21 11:07:20, Toon Verwaest wrote: > https://codereview.chromium.org/342453002/diff/40001/src/accessors.cc > File src/accessors.cc (right): > > https://codereview.chromium.org/342453002/diff/40001/src/accessors.cc#newcode184 ...
5 years, 3 months ago (2014-08-21 12:56:50 UTC) #13
wingo
Updated patch fixes { __proto__=arguments }[Symbol.iterator] = 10 case. It's pretty gross and I probably ...
5 years, 3 months ago (2014-08-21 15:33:04 UTC) #14
Toon Verwaest
https://codereview.chromium.org/342453002/diff/70008/src/accessors.cc File src/accessors.cc (right): https://codereview.chromium.org/342453002/diff/70008/src/accessors.cc#newcode180 src/accessors.cc:180: LookupIterator it(object, Utils::OpenHandle(*name)); What about just if (SetPropertyOnInstanceIfInherited(isolate, info, ...
5 years, 3 months ago (2014-08-22 11:01:02 UTC) #15
wingo
https://codereview.chromium.org/342453002/diff/70008/src/accessors.cc File src/accessors.cc (right): https://codereview.chromium.org/342453002/diff/70008/src/accessors.cc#newcode180 src/accessors.cc:180: LookupIterator it(object, Utils::OpenHandle(*name)); On 2014/08/22 11:01:02, Toon Verwaest wrote: ...
5 years, 3 months ago (2014-08-25 08:42:30 UTC) #16
Toon Verwaest
lgtm, thanks! (Note that you'll have to remove the .Check() for SetDataProperty since I changed ...
5 years, 3 months ago (2014-08-25 08:51:31 UTC) #17
wingo
5 years, 3 months ago (2014-08-25 09:12:34 UTC) #18
Message was sent while issue was closed.
Committed patchset #7 manually as 23341 (presubmit successful).

Powered by Google App Engine
This is Rietveld 408576698