Always finalize blocks after parsing, also for do-expressions

Always finalize blocks after parsing, also for do-expressions

Rather than finalizing after rewriting do-expressions, we rewrite in the
outer scope if the block scope was finalized. Rewriting do expressions
cannot introduce any new nodes that requires the block to stay around,
so finalizing before and after is equivalent. (Only a temporary is
introduced which always ends up in a ClosureScope)

BUG=v8:5209
R=rossberg@chromium.org, caitpotter88@gmail.com, adamk@chromium.org

Committed: https://crrev.com/f02018cee05f7e7c48aad7c3a5f4985a7c130d03
Cr-Commit-Position: refs/heads/master@{#38193}

[Toon Verwaest, 2016-07-21 12:30:52]

ptal

[Toon Verwaest, 2016-07-21 14:51:20]

The CQ bit was checked by verwaest@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-21 14:51:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-21 15:14:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-21 15:14:05]

Dry run: This issue passed the CQ dry run.

[adamk, 2016-07-21 17:15:41]

https://codereview.chromium.org/2167713004/diff/1/src/parsing/rewriter.cc
File src/parsing/rewriter.cc (right):

https://codereview.chromium.org/2167713004/diff/1/src/parsing/rewriter.cc#new...
src/parsing/rewriter.cc:368: Scope* scope = block->scope() == nullptr ?
outer_scope : block->scope();
Why should we ever use the block scope here? By your logic shouldn't it not
matter?

Could you rename the parameter to "scope" and replace this line with a DCHECK:

DCHECK(block->scope() == nullptr || (block->scope()->ClosureScope() ==
scope->ClosureScope()));

https://codereview.chromium.org/2167713004/diff/1/src/parsing/rewriter.h
File src/parsing/rewriter.h (right):

https://codereview.chromium.org/2167713004/diff/1/src/parsing/rewriter.h#newc...
src/parsing/rewriter.h:29: static bool Rewrite(Parser* parser, Scope*
outer_scope, DoExpression* expr,
Please add documentation to the comment saying what the scope is used for, to
make it clear to future readers why it's safe to call this with the outer scope.

[Toon Verwaest, 2016-07-22 09:35:07]

You're right, we just need the closure-scope. To make it obvious I just request
the closure scope directly as parameter. I also added additional checks that
closure scopes match, and a comment explaining why it needs to be passed in.

[adamk, 2016-07-22 23:00:17]

lgtm

[Toon Verwaest, 2016-08-01 07:50:58]

The CQ bit was checked by verwaest@chromium.org

[commit-bot: I haz the power, 2016-08-01 07:51:07]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-01 07:53:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-01 07:53:39]

Try jobs failed on following builders:
  v8_android_arm_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_android_arm_compile_rel/...)
  v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...)
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
  v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/10012)
  v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...)
  v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)
  v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...)
  v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/9955)

[Toon Verwaest, 2016-08-01 08:03:47]

The CQ bit was checked by verwaest@chromium.org

[Toon Verwaest, 2016-08-01 08:03:48]

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/2167713004/#ps40001
(title: "rebase")

[commit-bot: I haz the power, 2016-08-01 08:03:57]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-01 08:26:04]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-08-01 08:30:29]

Description was changed from

==========
Always finalize blocks after parsing, also for do-expressions

Rather than finalizing after rewriting do-expressions, we rewrite in the
outer scope if the block scope was finalized. Rewriting do expressions
cannot introduce any new nodes that requires the block to stay around,
so finalizing before and after is equivalent. (Only a temporary is
introduced which always ends up in a ClosureScope)

BUG=v8:5209
R=rossberg@chromium.org, caitpotter88@gmail.com, adamk@chromium.org
==========

to

==========
Always finalize blocks after parsing, also for do-expressions

Rather than finalizing after rewriting do-expressions, we rewrite in the
outer scope if the block scope was finalized. Rewriting do expressions
cannot introduce any new nodes that requires the block to stay around,
so finalizing before and after is equivalent. (Only a temporary is
introduced which always ends up in a ClosureScope)

BUG=v8:5209
R=rossberg@chromium.org, caitpotter88@gmail.com, adamk@chromium.org

Committed: https://crrev.com/f02018cee05f7e7c48aad7c3a5f4985a7c130d03
Cr-Commit-Position: refs/heads/master@{#38193}
==========

[commit-bot: I haz the power, 2016-08-01 08:30:30]

Patchset 3 (id:??) landed as
https://crrev.com/f02018cee05f7e7c48aad7c3a5f4985a7c130d03
Cr-Commit-Position: refs/heads/master@{#38193}