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

Issue 1695393003: [es6] Implement for-of iterator finalization (Closed)

Created:
4 years, 10 months ago by rossberg
Modified:
4 years, 10 months ago
CC:
neis, v8-reviews_googlegroups.com
Base URL:
https://chromium.googlesource.com/v8/v8.git@master
Target Ref:
refs/pending/heads/master
Project:
v8
Visibility:
Public.

Description

[es6] Implement for-of iterator finalization Implements iterator finalisation by desugaring for-of loops with an additional try-finally wrapper. See comment in parser.cc for details. Also improved some AST printing facilities while there. @Ross, I had to disable the bytecode generation test for for-of, because it got completely out of hand after this change (the new bytecode has 150+ lines). See the TODO that I assigned to you. Patch set 1 is WIP patch by Georg (http://crrev.com/1695583003), patch set 2 relative changes. @Georg, FYI, I changed the following: - Moved try-finally out of the loop body, for performance, and in order to be able to handle `continue` correctly. - Fixed scope management in ParseForStatement, which was the cause for the variable allocation failure. - Fixed pre-existing zone initialisation bug in rewriter, which caused the crashes. - Enabled all tests, adjusted a few others, added a couple more. BUG=v8:2214 LOG=Y Committed: https://crrev.com/cb1bf4af3c961093e9a75a22089f78daf7959304 Cr-Commit-Position: refs/heads/master@{#34111}

Patch Set 1 #

Patch Set 2 : Final version #

Patch Set 3 : Remove some debug left-overs #

Total comments: 23

Patch Set 4 : Fixed bug, addressed comments #

Patch Set 5 : Epsilon #

Total comments: 3

Patch Set 6 : Remove double empty lines #

Patch Set 7 : Disable iterator test for Ignition #

Unified diffs Side-by-side diffs Delta from patch set Stats (+880 lines, -119 lines) Patch
M src/ast/ast.h View 1 5 chunks +10 lines, -2 lines 0 comments Download
M src/ast/ast.cc View 1 1 chunk +2 lines, -2 lines 0 comments Download
M src/ast/ast-value-factory.h View 1 chunk +1 line, -0 lines 0 comments Download
M src/ast/prettyprinter.h View 1 1 chunk +3 lines, -0 lines 0 comments Download
M src/ast/prettyprinter.cc View 1 3 chunks +37 lines, -20 lines 0 comments Download
M src/ast/scopes.cc View 2 chunks +2 lines, -2 lines 0 comments Download
M src/bootstrapper.cc View 1 2 3 2 chunks +2 lines, -0 lines 0 comments Download
M src/flag-definitions.h View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M src/messages.h View 1 chunk +1 line, -0 lines 0 comments Download
M src/parsing/parser.h View 1 2 3 2 chunks +8 lines, -2 lines 0 comments Download
M src/parsing/parser.cc View 1 2 3 4 5 16 chunks +441 lines, -81 lines 0 comments Download
M src/parsing/rewriter.cc View 1 1 chunk +1 line, -0 lines 0 comments Download
M test/cctest/interpreter/test-bytecode-generator.cc View 1 1 chunk +3 lines, -1 line 0 comments Download
M test/mjsunit/harmony/destructuring.js View 1 2 3 1 chunk +2 lines, -2 lines 0 comments Download
M test/mjsunit/harmony/generators.js View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
A test/mjsunit/harmony/iterator-close.js View 1 2 3 1 chunk +364 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
M test/mjsunit/strong/for-in.js View 1 1 chunk +0 lines, -6 lines 0 comments Download

Messages

Total messages: 26 (12 generated)
rossberg
PTAL
4 years, 10 months ago (2016-02-16 16:29:19 UTC) #4
rossberg
Georg, FYI.
4 years, 10 months ago (2016-02-16 16:59:39 UTC) #5
Dan Ehrenberg
https://codereview.chromium.org/1695393003/diff/40001/src/ast/prettyprinter.cc File src/ast/prettyprinter.cc (right): https://codereview.chromium.org/1695393003/diff/40001/src/ast/prettyprinter.cc#newcode1568 src/ast/prettyprinter.cc:1568: break; Minor: Did git cl format add these indents? ...
4 years, 10 months ago (2016-02-16 21:18:30 UTC) #7
mvstanton
https://codereview.chromium.org/1695393003/diff/40001/src/parsing/parser.cc File src/parsing/parser.cc (right): https://codereview.chromium.org/1695393003/diff/40001/src/parsing/parser.cc#newcode6475 src/parsing/parser.cc:6475: Token::EQ_STRICT, type_of, function_literal, nopos); On 2016/02/16 21:18:30, Dan Ehrenberg ...
4 years, 10 months ago (2016-02-16 22:55:46 UTC) #10
rossberg
Addressed comments. Also fixed behaviour wrt exceptions produced by the .next method, which should _not_ ...
4 years, 10 months ago (2016-02-17 12:54:45 UTC) #11
Dan Ehrenberg
Oh yeah, I remember now Georg explaining that he needed a try/catch on every iteration ...
4 years, 10 months ago (2016-02-17 15:42:01 UTC) #12
rossberg
I change the completion flag between .next call and loop body. https://codereview.chromium.org/1695393003/diff/80001/src/parsing/parser.cc File src/parsing/parser.cc (right): ...
4 years, 10 months ago (2016-02-17 16:04:07 UTC) #13
Dan Ehrenberg
lgtm
4 years, 10 months ago (2016-02-17 16:09:51 UTC) #14
Dan Ehrenberg
lgtm
4 years, 10 months ago (2016-02-17 16:10:01 UTC) #15
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1695393003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1695393003/100001
4 years, 10 months ago (2016-02-17 17:14:29 UTC) #17
commit-bot: I haz the power
Try jobs failed on following builders: v8_linux_arm64_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel/builds/15474)
4 years, 10 months ago (2016-02-17 17:33:33 UTC) #19
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1695393003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1695393003/120001
4 years, 10 months ago (2016-02-18 10:28:10 UTC) #22
commit-bot: I haz the power
Committed patchset #7 (id:120001)
4 years, 10 months ago (2016-02-18 10:49:11 UTC) #24
commit-bot: I haz the power
4 years, 10 months ago (2016-02-18 10:50:03 UTC) #26
Message was sent while issue was closed.
Patchset 7 (id:??) landed as
https://crrev.com/cb1bf4af3c961093e9a75a22089f78daf7959304
Cr-Commit-Position: refs/heads/master@{#34111}

Powered by Google App Engine
This is Rietveld 408576698