Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(244)

Issue 2407423002: [modules] Implement @@iterator on namespace objects. (Closed)

Created:
4 years, 2 months ago by neis
Modified:
4 years, 2 months ago
CC:
v8-reviews_googlegroups.com, Hannes Payer (out of office), ulan, rmcilroy
Target Ref:
refs/pending/heads/master
Project:
v8
Visibility:
Public.

Description

[modules] Implement @@iterator on namespace objects. As part of this, introduce a new JSObject for iterating over the elements of a FixedArray. R=adamk@chromium.org,bmeurer@chromium.org TBR=ulan@chromium.org BUG=v8:1569 Committed: https://crrev.com/dafe6867f33ab95cb113159e9088da905c8de37b Cr-Commit-Position: refs/heads/master@{#40265}

Patch Set 1 #

Total comments: 10

Patch Set 2 : Address review comments. #

Patch Set 3 : Rebase. #

Patch Set 4 : Fix the JSFixedArrayIterator setup. #

Patch Set 5 : Update test262.status. #

Total comments: 2

Patch Set 6 : Rebase and rebaseline bytecode expectations. #

Patch Set 7 : Rename next to initial_next and kHeaderSize to kSize. #

Patch Set 8 : Rename kSize to kHeadersize again. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+233 lines, -33 lines) Patch
M src/ast/ast-types.cc View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M src/bootstrapper.cc View 1 2 3 4 5 6 7 2 chunks +37 lines, -9 lines 0 comments Download
M src/builtins/builtins.h View 1 2 3 4 5 1 chunk +4 lines, -1 line 0 comments Download
M src/builtins/builtins-iterator.cc View 1 2 3 4 5 6 2 chunks +52 lines, -1 line 0 comments Download
M src/compiler/types.cc View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M src/contexts.h View 1 2 3 4 5 1 chunk +1 line, -0 lines 0 comments Download
M src/factory.h View 1 2 1 chunk +3 lines, -0 lines 0 comments Download
M src/factory.cc View 1 2 3 4 5 6 1 chunk +21 lines, -0 lines 0 comments Download
M src/heap/objects-visiting.cc View 1 chunk +1 line, -0 lines 0 comments Download
M src/objects.h View 1 2 3 4 5 6 7 5 chunks +35 lines, -0 lines 0 comments Download
M src/objects-body-descriptors-inl.h View 1 chunk +1 line, -0 lines 0 comments Download
M src/objects-debug.cc View 1 2 3 4 5 6 2 chunks +13 lines, -0 lines 0 comments Download
M src/objects-inl.h View 1 2 3 4 5 6 7 4 chunks +8 lines, -0 lines 0 comments Download
M src/objects-printer.cc View 1 2 3 4 5 6 2 chunks +11 lines, -0 lines 0 comments Download
M test/cctest/interpreter/bytecode_expectations/CallRuntime.golden View 1 2 3 4 5 1 chunk +1 line, -1 line 0 comments Download
M test/mjsunit/modules-namespace1.js View 1 2 3 3 chunks +35 lines, -10 lines 0 comments Download
M test/mjsunit/modules-namespace2.js View 1 chunk +3 lines, -3 lines 0 comments Download
M test/test262/test262.status View 1 2 3 4 1 chunk +5 lines, -8 lines 0 comments Download

Messages

Total messages: 39 (19 generated)
neis
4 years, 2 months ago (2016-10-11 18:32:57 UTC) #1
neis
+bmeurer for the awesomeness in builtins-iterator.cc. https://codereview.chromium.org/2407423002/diff/1/src/bootstrapper.cc File src/bootstrapper.cc (right): https://codereview.chromium.org/2407423002/diff/1/src/bootstrapper.cc#newcode2790 src/bootstrapper.cc:2790: map->SetConstructor(native_context->object_function()); I don't ...
4 years, 2 months ago (2016-10-11 18:36:17 UTC) #3
neis
+bmeurer for the awesomeness in builtins-iterator.cc.
4 years, 2 months ago (2016-10-11 18:36:18 UTC) #4
adamk
lgtm other than the BuiltinFrame code (which I'm not at all familiar with). https://codereview.chromium.org/2407423002/diff/1/src/builtins/builtins-iterator.cc File ...
4 years, 2 months ago (2016-10-11 19:42:53 UTC) #6
Benedikt Meurer
Nice! LGTM once comments addressed. https://codereview.chromium.org/2407423002/diff/1/src/bootstrapper.cc File src/bootstrapper.cc (right): https://codereview.chromium.org/2407423002/diff/1/src/bootstrapper.cc#newcode2171 src/bootstrapper.cc:2171: // TODO(neis): Is this ...
4 years, 2 months ago (2016-10-12 03:24:06 UTC) #7
adamk
https://codereview.chromium.org/2407423002/diff/1/src/bootstrapper.cc File src/bootstrapper.cc (right): https://codereview.chromium.org/2407423002/diff/1/src/bootstrapper.cc#newcode2171 src/bootstrapper.cc:2171: // TODO(neis): Is this really supposed to be writable? ...
4 years, 2 months ago (2016-10-12 16:21:47 UTC) #8
adamk
I've been discussing some spec issues about this particular feature with folks on IRC (mozilla.org ...
4 years, 2 months ago (2016-10-12 17:29:42 UTC) #9
Benedikt Meurer
On 2016/10/12 17:29:42, adamk wrote: > I've been discussing some spec issues about this particular ...
4 years, 2 months ago (2016-10-13 03:32:19 UTC) #10
neis
https://codereview.chromium.org/2407423002/diff/1/src/builtins/builtins-iterator.cc File src/builtins/builtins-iterator.cc (right): https://codereview.chromium.org/2407423002/diff/1/src/builtins/builtins-iterator.cc#newcode47 src/builtins/builtins-iterator.cc:47: } On 2016/10/12 03:24:05, Benedikt Meurer wrote: > Adam ...
4 years, 2 months ago (2016-10-13 07:37:21 UTC) #11
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2407423002/20001
4 years, 2 months ago (2016-10-13 07:37:52 UTC) #14
commit-bot: I haz the power
Try jobs failed on following builders: v8_presubmit on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/26335)
4 years, 2 months ago (2016-10-13 07:43:03 UTC) #16
neis
It turns out my JSFixedArrayIterator object was broken (e.g. adding a new property to it ...
4 years, 2 months ago (2016-10-13 11:41:04 UTC) #21
neis
On 2016/10/13 11:41:04, neis wrote: > It turns out my JSFixedArrayIterator object was broken (e.g. ...
4 years, 2 months ago (2016-10-13 11:42:40 UTC) #23
Igor Sheludko
lgtm with nits: https://codereview.chromium.org/2407423002/diff/120001/src/objects.h File src/objects.h (right): https://codereview.chromium.org/2407423002/diff/120001/src/objects.h#newcode10458 src/objects.h:10458: DECL_ACCESSORS(next, JSFunction) Maybe call it initial_next ...
4 years, 2 months ago (2016-10-13 11:58:27 UTC) #26
Benedikt Meurer
Still LGTM. Magic 1 for the win!
4 years, 2 months ago (2016-10-13 12:18:10 UTC) #27
neis
> https://codereview.chromium.org/2407423002/diff/120001/src/objects.h#newcode10458 > src/objects.h:10458: DECL_ACCESSORS(next, JSFunction) > Maybe call it initial_next to avoid confusion with ...
4 years, 2 months ago (2016-10-13 12:18:37 UTC) #28
Igor Sheludko
On 2016/10/13 12:18:37, neis wrote: > Some objects do define kHeaderSize instead of kSize and ...
4 years, 2 months ago (2016-10-13 12:47:37 UTC) #31
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2407423002/180001
4 years, 2 months ago (2016-10-13 12:57:05 UTC) #35
commit-bot: I haz the power
Committed patchset #8 (id:180001)
4 years, 2 months ago (2016-10-13 13:34:58 UTC) #37
commit-bot: I haz the power
4 years, 2 months ago (2016-10-13 13:35:12 UTC) #39
Message was sent while issue was closed.
Patchset 8 (id:??) landed as
https://crrev.com/dafe6867f33ab95cb113159e9088da905c8de37b
Cr-Commit-Position: refs/heads/master@{#40265}

Powered by Google App Engine
This is Rietveld 408576698