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

Issue 1399893002: [es7] implement |do| expressions proposal (Closed)

Created:
3 years, 9 months ago by caitp (gmail)
Modified:
3 years ago
CC:
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

[es7] implement |do| expressions proposal Adds an implementation of "do expression" parsing (https://webcache.googleusercontent.com/search?q=cache:MIGALjqPDNgJ:wiki.ecmascript.org/doku.php%3Fid%3Dstrawman:do_expressions+&cd=1&hl=en&ct=clnk&gl=us). This feature provides a way to evaluate a block of statements within an expression context, producing the resulting completion value. This is very helpful for implementing certain language features via desugaring. BUG=v8:4488 LOG=N R=adamk@chromium.org, bmeurer@chromium.org, rossberg@chromium.org, wingo@igalia.com Committed: https://crrev.com/b6596aa73b069aaa6e52291c3756b0e0f0e483b6 Cr-Commit-Position: refs/heads/master@{#31428}

Patch Set 1 #

Patch Set 2 : Remove CompletionStatement AST node #

Patch Set 3 : Parse as PrimaryExpression, per draft proposal on wiki #

Patch Set 4 : fix AST numbering issue + add simple TF impl #

Total comments: 16

Patch Set 5 : Build ontop of Block node, add crankshaft support, refactor some stuff #

Patch Set 6 : Some cleanup #

Total comments: 15

Patch Set 7 : rebased #

Patch Set 8 : Fix bailout handling in crankshaft visitor + more tests #

Patch Set 9 : Rename flag to --harmony_do_expressions #

Patch Set 10 : Fix nits/typer + add a test with explicit opt/deopt #

Patch Set 11 : Less code duplication in Rewriter #

Total comments: 14

Patch Set 12 : Add more test cases + some TODOs #

Total comments: 9

Patch Set 13 : Rebased #

Patch Set 14 : Remove unneeded change from preparser #

Patch Set 15 : Add a TODO test for picking the right declaration scope when declarations used in do expressions in… #

Patch Set 16 : More testing #

Patch Set 17 : Rebased #

Patch Set 18 : Disable CrankShaft #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+524 lines, -14 lines) Patch
M src/ast.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 4 chunks +35 lines, -1 line 1 comment Download
M src/ast-expression-visitor.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +6 lines, -0 lines 0 comments Download
M src/ast-literal-reindexer.cc View 1 1 chunk +5 lines, -0 lines 0 comments Download
M src/ast-numbering.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +9 lines, -0 lines 0 comments Download
M src/bailout-reason.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +1 line, -0 lines 0 comments Download
M src/bootstrapper.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 2 chunks +2 lines, -0 lines 0 comments Download
M src/compiler/ast-graph-builder.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +6 lines, -0 lines 0 comments Download
M src/compiler/ast-loop-assignment-analyzer.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +6 lines, -0 lines 0 comments Download
M src/crankshaft/hydrogen.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +5 lines, -0 lines 0 comments Download
M src/crankshaft/typing.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +7 lines, -0 lines 0 comments Download
M src/flag-definitions.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +2 lines, -1 line 0 comments Download
M src/full-codegen/full-codegen.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +9 lines, -0 lines 0 comments Download
M src/interpreter/bytecode-generator.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +5 lines, -0 lines 0 comments Download
M src/parser.h View 1 2 3 4 5 5 chunks +13 lines, -0 lines 0 comments Download
M src/parser.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 4 chunks +21 lines, -0 lines 0 comments Download
M src/pattern-rewriter.cc View 1 1 chunk +1 line, -0 lines 0 comments Download
M src/preparser.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 9 chunks +24 lines, -1 line 0 comments Download
M src/preparser.cc View 1 2 3 4 1 chunk +17 lines, -0 lines 0 comments Download
M src/prettyprinter.cc View 1 2 3 4 5 6 7 8 9 10 11 12 3 chunks +16 lines, -0 lines 0 comments Download
M src/rewriter.h View 2 chunks +8 lines, -0 lines 0 comments Download
M src/rewriter.cc View 1 2 3 4 5 6 7 8 9 10 11 12 4 chunks +48 lines, -11 lines 0 comments Download
M src/typing-asm.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +5 lines, -0 lines 0 comments Download
A test/mjsunit/harmony/do-expressions.js View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +273 lines, -0 lines 0 comments Download

Messages

Total messages: 73 (15 generated)
caitp (gmail)
This is a work in progress, first sort of checkpoint in the prototyping of do-expressions. ...
3 years, 9 months ago (2015-10-11 03:10:23 UTC) #1
caitp (gmail)
On 2015/10/11 03:10:23, caitp wrote: > This is a work in progress, first sort of ...
3 years, 9 months ago (2015-10-12 13:03:56 UTC) #5
rossberg
Excellent! https://codereview.chromium.org/1399893002/diff/100001/src/ast.cc File src/ast.cc (right): https://codereview.chromium.org/1399893002/diff/100001/src/ast.cc#newcode1131 src/ast.cc:1131: Variable* AstNodeFactory::NewTemporaryVar(Scope* scope, This function seems rather redundant. ...
3 years, 9 months ago (2015-10-12 13:52:40 UTC) #6
caitp (gmail)
Some cleanup Few things left untouched: - the flag name is still the same, but ...
3 years, 9 months ago (2015-10-12 18:23:00 UTC) #7
adamk
https://codereview.chromium.org/1399893002/diff/100001/src/rewriter.cc File src/rewriter.cc (left): https://codereview.chromium.org/1399893002/diff/100001/src/rewriter.cc#oldcode75 src/rewriter.cc:75: DEFINE_AST_VISITOR_SUBCLASS_MEMBERS(); On 2015/10/12 18:23:00, caitp wrote: > On 2015/10/12 ...
3 years, 9 months ago (2015-10-12 19:02:21 UTC) #8
rossberg
https://codereview.chromium.org/1399893002/diff/100001/src/compiler/ast-graph-builder.cc File src/compiler/ast-graph-builder.cc (right): https://codereview.chromium.org/1399893002/diff/100001/src/compiler/ast-graph-builder.cc#newcode1674 src/compiler/ast-graph-builder.cc:1674: if (expr->result()->is_assigned()) { On 2015/10/12 18:22:59, caitp wrote: > ...
3 years, 9 months ago (2015-10-13 10:44:33 UTC) #9
caitp (gmail)
https://codereview.chromium.org/1399893002/diff/100001/src/flag-definitions.h File src/flag-definitions.h (right): https://codereview.chromium.org/1399893002/diff/100001/src/flag-definitions.h#newcode190 src/flag-definitions.h:190: DEFINE_BOOL(do_expression_parsing, false, "do expression parsing") On 2015/10/13 10:44:32, rossberg ...
3 years, 9 months ago (2015-10-13 15:06:00 UTC) #10
adamk
If you sync to https://chromium.googlesource.com/v8/v8/+/d317145d05f25232f8c45f5b8ed98e2df6610afb, it should be trivial to add a new InitializeAstVisitor() method ...
3 years, 9 months ago (2015-10-14 13:06:26 UTC) #11
caitp (gmail)
On 2015/10/14 13:06:26, adamk wrote: > If you sync to > https://chromium.googlesource.com/v8/v8/+/d317145d05f25232f8c45f5b8ed98e2df6610afb, > it should ...
3 years, 9 months ago (2015-10-14 13:37:19 UTC) #12
rossberg
LGTM https://codereview.chromium.org/1399893002/diff/140001/src/typing.cc File src/typing.cc (right): https://codereview.chromium.org/1399893002/diff/140001/src/typing.cc#newcode353 src/typing.cc:353: RECURSE(VisitVariableProxy(expr->result())); On 2015/10/13 15:06:00, caitp wrote: > On ...
3 years, 9 months ago (2015-10-15 09:58:27 UTC) #13
adamk
Definitely would like more tests too: var and function declarations being the ones I'm most ...
3 years, 9 months ago (2015-10-15 10:59:33 UTC) #14
caitp (gmail)
https://codereview.chromium.org/1399893002/diff/140001/test/mjsunit/internal-do-expressions.js File test/mjsunit/internal-do-expressions.js (right): https://codereview.chromium.org/1399893002/diff/140001/test/mjsunit/internal-do-expressions.js#newcode14 test/mjsunit/internal-do-expressions.js:14: function TestBasic() { On 2015/10/15 09:58:27, rossberg wrote: > ...
3 years, 9 months ago (2015-10-15 11:24:17 UTC) #15
adamk
https://codereview.chromium.org/1399893002/diff/240001/src/ast-literal-reindexer.cc File src/ast-literal-reindexer.cc (right): https://codereview.chromium.org/1399893002/diff/240001/src/ast-literal-reindexer.cc#newcode47 src/ast-literal-reindexer.cc:47: // TODO(caitp): literals in do expressions need re-indexing too. ...
3 years, 9 months ago (2015-10-15 12:01:25 UTC) #16
caitp (gmail)
https://codereview.chromium.org/1399893002/diff/240001/src/preparser.cc File src/preparser.cc (right): https://codereview.chromium.org/1399893002/diff/240001/src/preparser.cc#newcode1248 src/preparser.cc:1248: Scope* block_scope = NewScope(scope_, BLOCK_SCOPE); On 2015/10/15 12:01:25, adamk ...
3 years, 9 months ago (2015-10-15 12:34:07 UTC) #17
adamk
https://codereview.chromium.org/1399893002/diff/240001/src/preparser.h File src/preparser.h (right): https://codereview.chromium.org/1399893002/diff/240001/src/preparser.h#newcode2428 src/preparser.h:2428: if (token == Token::DO && allow_harmony_do_expressions()) { On 2015/10/15 ...
3 years, 9 months ago (2015-10-15 12:42:27 UTC) #18
caitp (gmail)
On 2015/10/15 09:58:27, rossberg wrote: > LGTM > > https://codereview.chromium.org/1399893002/diff/140001/src/typing.cc > File src/typing.cc (right): > ...
3 years, 9 months ago (2015-10-15 14:18:09 UTC) #19
adamk
On 2015/10/15 14:18:09, caitp wrote: > On 2015/10/15 09:58:27, rossberg wrote: > > LGTM > ...
3 years, 9 months ago (2015-10-15 14:21:58 UTC) #20
caitp (gmail)
On 2015/10/15 14:21:58, adamk wrote: > On 2015/10/15 14:18:09, caitp wrote: > > On 2015/10/15 ...
3 years, 9 months ago (2015-10-15 14:26:24 UTC) #21
caitp (gmail)
I've added a bunch more tests (though more are needed for sure), and added some ...
3 years, 9 months ago (2015-10-15 16:00:26 UTC) #22
adamk
lgtm as a first shot. Still wishing var declarations weren't allowed :)
3 years, 9 months ago (2015-10-15 17:06:56 UTC) #23
caitp (gmail)
On 2015/10/15 17:06:56, adamk wrote: > lgtm as a first shot. Still wishing var declarations ...
3 years, 9 months ago (2015-10-15 17:08:22 UTC) #24
adamk
Yeah, sorry, I realize it'd have to be part of the proposal. Just wishful thinking ...
3 years, 9 months ago (2015-10-15 17:09:16 UTC) #25
caitp (gmail)
On 2015/10/15 17:09:16, adamk wrote: > Yeah, sorry, I realize it'd have to be part ...
3 years, 9 months ago (2015-10-15 17:10:17 UTC) #26
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1399893002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1399893002/260001
3 years, 9 months ago (2015-10-15 17:10:32 UTC) #29
commit-bot: I haz the power
Try jobs failed on following builders: v8_linux64_asan_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel/builds/8976) v8_linux_gcc_compile_rel on tryserver.v8 (JOB_FAILED, ...
3 years, 9 months ago (2015-10-15 17:11:30 UTC) #31
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1399893002/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1399893002/280001
3 years, 9 months ago (2015-10-15 17:21:18 UTC) #34
commit-bot: I haz the power
Try jobs failed on following builders: v8_presubmit on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/6721)
3 years, 9 months ago (2015-10-15 17:23:24 UTC) #36
caitp (gmail)
On 2015/10/15 17:23:24, commit-bot: I haz the power wrote: > Try jobs failed on following ...
3 years, 9 months ago (2015-10-15 17:24:16 UTC) #37
rossberg
Benedikt, can you look at the TF parts? https://codereview.chromium.org/1399893002/diff/260001/test/mjsunit/harmony/do-expressions.js File test/mjsunit/harmony/do-expressions.js (right): https://codereview.chromium.org/1399893002/diff/260001/test/mjsunit/harmony/do-expressions.js#newcode55 test/mjsunit/harmony/do-expressions.js:55: for ...
3 years, 9 months ago (2015-10-16 12:09:41 UTC) #38
caitp (gmail)
https://codereview.chromium.org/1399893002/diff/260001/test/mjsunit/harmony/do-expressions.js File test/mjsunit/harmony/do-expressions.js (right): https://codereview.chromium.org/1399893002/diff/260001/test/mjsunit/harmony/do-expressions.js#newcode210 test/mjsunit/harmony/do-expressions.js:210: function TestHoisting() { On 2015/10/16 12:09:40, rossberg wrote: > ...
3 years, 9 months ago (2015-10-16 12:21:00 UTC) #39
caitp (gmail)
On 2015/10/16 12:21:00, caitp wrote: > https://codereview.chromium.org/1399893002/diff/260001/test/mjsunit/harmony/do-expressions.js > File test/mjsunit/harmony/do-expressions.js (right): > > https://codereview.chromium.org/1399893002/diff/260001/test/mjsunit/harmony/do-expressions.js#newcode210 > ...
3 years, 9 months ago (2015-10-16 12:21:53 UTC) #40
rossberg
https://codereview.chromium.org/1399893002/diff/260001/test/mjsunit/harmony/do-expressions.js File test/mjsunit/harmony/do-expressions.js (right): https://codereview.chromium.org/1399893002/diff/260001/test/mjsunit/harmony/do-expressions.js#newcode210 test/mjsunit/harmony/do-expressions.js:210: function TestHoisting() { On 2015/10/16 12:21:00, caitp wrote: > ...
3 years, 9 months ago (2015-10-16 12:37:02 UTC) #41
caitp (gmail)
https://codereview.chromium.org/1399893002/diff/260001/test/mjsunit/harmony/do-expressions.js File test/mjsunit/harmony/do-expressions.js (right): https://codereview.chromium.org/1399893002/diff/260001/test/mjsunit/harmony/do-expressions.js#newcode55 test/mjsunit/harmony/do-expressions.js:55: for (var i = 0; i < 10; ++i) ...
3 years, 9 months ago (2015-10-16 13:12:19 UTC) #42
caitp (gmail)
https://codereview.chromium.org/1399893002/diff/260001/test/mjsunit/harmony/do-expressions.js File test/mjsunit/harmony/do-expressions.js (right): https://codereview.chromium.org/1399893002/diff/260001/test/mjsunit/harmony/do-expressions.js#newcode210 test/mjsunit/harmony/do-expressions.js:210: function TestHoisting() { On 2015/10/16 12:37:02, rossberg wrote: > ...
3 years, 9 months ago (2015-10-16 13:23:03 UTC) #43
rossberg
https://codereview.chromium.org/1399893002/diff/260001/test/mjsunit/harmony/do-expressions.js File test/mjsunit/harmony/do-expressions.js (right): https://codereview.chromium.org/1399893002/diff/260001/test/mjsunit/harmony/do-expressions.js#newcode210 test/mjsunit/harmony/do-expressions.js:210: function TestHoisting() { On 2015/10/16 13:23:03, caitp wrote: > ...
3 years, 9 months ago (2015-10-16 13:28:28 UTC) #44
Benedikt Meurer
LGTM for compiler.
3 years, 9 months ago (2015-10-20 15:41:57 UTC) #45
caitp (gmail)
On 2015/10/20 15:41:57, Benedikt Meurer wrote: > LGTM for compiler. Thanks, I appreciate it. CQing, ...
3 years, 9 months ago (2015-10-20 19:56:57 UTC) #46
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1399893002/360001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1399893002/360001
3 years, 9 months ago (2015-10-20 19:57:12 UTC) #49
commit-bot: I haz the power
Try jobs failed on following builders: v8_mac_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel/builds/10909)
3 years, 9 months ago (2015-10-20 20:11:06 UTC) #51
caitp (gmail)
On 2015/10/20 20:11:06, commit-bot: I haz the power wrote: > Try jobs failed on following ...
3 years, 9 months ago (2015-10-20 20:13:05 UTC) #52
caitp (gmail)
On 2015/10/20 20:13:05, caitp wrote: > On 2015/10/20 20:11:06, commit-bot: I haz the power wrote: ...
3 years, 9 months ago (2015-10-20 21:45:59 UTC) #53
caitp (gmail)
On 2015/10/20 21:45:59, caitp wrote: > On 2015/10/20 20:13:05, caitp wrote: > > On 2015/10/20 ...
3 years, 9 months ago (2015-10-20 22:17:26 UTC) #54
caitp (gmail)
On 2015/10/20 22:17:26, caitp wrote: > On 2015/10/20 21:45:59, caitp wrote: > > On 2015/10/20 ...
3 years, 9 months ago (2015-10-20 22:53:55 UTC) #55
Benedikt Meurer (Google)
On 2015/10/20 22:53:55, caitp wrote: > On 2015/10/20 22:17:26, caitp wrote: > > On 2015/10/20 ...
3 years, 9 months ago (2015-10-21 02:15:21 UTC) #56
caitp (gmail)
On 2015/10/21 02:15:21, Benedikt Meurer (Google) wrote: > On 2015/10/20 22:53:55, caitp wrote: > > ...
3 years, 9 months ago (2015-10-21 02:16:38 UTC) #57
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1399893002/380001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1399893002/380001
3 years, 9 months ago (2015-10-21 02:31:59 UTC) #60
commit-bot: I haz the power
Committed patchset #18 (id:380001)
3 years, 9 months ago (2015-10-21 02:55:54 UTC) #61
commit-bot: I haz the power
Patchset 18 (id:??) landed as https://crrev.com/b6596aa73b069aaa6e52291c3756b0e0f0e483b6 Cr-Commit-Position: refs/heads/master@{#31428}
3 years, 9 months ago (2015-10-21 02:58:43 UTC) #62
adamk
While disabling this for Crankshaft seems fine as a first step, I'd worry about leaving ...
3 years, 9 months ago (2015-10-21 09:04:36 UTC) #63
caitp (gmail)
On 2015/10/21 09:04:36, adamk wrote: > While disabling this for Crankshaft seems fine as a ...
3 years, 9 months ago (2015-10-21 10:16:17 UTC) #64
Benedikt Meurer
I'd be really in favor of not adding anything ES6ish/newish to Crankshaft at all. We ...
3 years, 9 months ago (2015-10-21 10:30:14 UTC) #65
caitp (gmail)
On 2015/10/21 10:30:14, Benedikt Meurer wrote: > I'd be really in favor of not adding ...
3 years, 9 months ago (2015-10-21 10:35:04 UTC) #66
rossberg
On 2015/10/21 10:16:17, caitp wrote: > On 2015/10/21 09:04:36, adamk wrote: > > While disabling ...
3 years, 9 months ago (2015-10-21 10:40:21 UTC) #67
caitp (gmail)
On 2015/10/21 10:40:21, rossberg wrote: > On 2015/10/21 10:16:17, caitp wrote: > > On 2015/10/21 ...
3 years, 9 months ago (2015-10-21 10:47:33 UTC) #68
rossberg
On 2015/10/21 10:47:33, caitp wrote: > I'm not totally sure why this isn't crashing spectacularly ...
3 years, 9 months ago (2015-10-21 10:51:50 UTC) #69
marja
This CL is a bit old, but I bumped into this question... maybe you people ...
3 years ago (2016-07-13 12:08:05 UTC) #71
caitp
On 2016/07/13 12:08:05, marja wrote: > This CL is a bit old, but I bumped ...
3 years ago (2016-07-13 21:45:46 UTC) #72
marja
3 years ago (2016-07-14 09:57:36 UTC) #73
Message was sent while issue was closed.
Hmm, I don't understand this. Where's the delayed rewriting desugaring
happening? ParseDoExpression calls Rewriter::Rewrite(Parser* parser,
DoExpression* expr, AstValueFactory* factory) but that's not delayed... And
where do we even get the DoExpression from, since its parent AST nodes will be
in the temp zone and thus discarded.

Afaics, it's potentially worrying that the Block* of the DoExpression will
reside in the temp zone which will then be discarded, so the DoExpression refers
to dead memory.

Powered by Google App Engine
This is Rietveld 408576698