[parsing] Fix bug in completion value of try-finally.

[parsing] Fix bug in completion value of try-finally.

R=adamk@chromium.org, verwaest@chromium.org
BUG=v8:5698
Committed: https://crrev.com/67f1969a078a30906246e6b6d94d7468de9710f9
Cr-Commit-Position: refs/heads/master@{#41495}

[adamk, 2016-11-30 18:13:19]

lgtm, but I'd like Toon to take a look too since he wrote this version of the
code...

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

https://codereview.chromium.org/2537413003/diff/1/src/parsing/rewriter.cc#new...
src/parsing/rewriter.cc:246: bool set_after = is_set_;
Don't need this anymore

https://codereview.chromium.org/2537413003/diff/1/src/parsing/rewriter.cc#new...
src/parsing/rewriter.cc:247: // Only set result before a 'break' or 'continue'.
This comment sounds redundant with the longer comment below. Maybe just drop the
second one?

[neis, 2016-12-01 06:57:07]

I need to think about this a little more. This morning I'm not convinced this
change is correct in general.

[neis, 2016-12-01 10:22:44]

Ok, I think it's fine.

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

https://codereview.chromium.org/2537413003/diff/1/src/parsing/rewriter.cc#new...
src/parsing/rewriter.cc:246: bool set_after = is_set_;
On 2016/11/30 18:13:19, adamk wrote:
> Don't need this anymore

Acknowledged.

https://codereview.chromium.org/2537413003/diff/1/src/parsing/rewriter.cc#new...
src/parsing/rewriter.cc:247: // Only set result before a 'break' or 'continue'.
On 2016/11/30 18:13:19, adamk wrote:
> This comment sounds redundant with the longer comment below. Maybe just drop
the
> second one?

Not sure what you mean.

The comment here justifies why it's okay to set is_set_ to true before rewriting
the finally block.

The 4 line comment below talks about saving and restoring .result, I don't want
to remove that.

[adamk, 2016-12-01 18:49:31]

still lgtm

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

https://codereview.chromium.org/2537413003/diff/1/src/parsing/rewriter.cc#new...
src/parsing/rewriter.cc:247: // Only set result before a 'break' or 'continue'.
On 2016/12/01 10:22:44, neis wrote:
> On 2016/11/30 18:13:19, adamk wrote:
> > This comment sounds redundant with the longer comment below. Maybe just drop
> the
> > second one?
> 
> Not sure what you mean.
> 
> The comment here justifies why it's okay to set is_set_ to true before
rewriting
> the finally block.
> 
> The 4 line comment below talks about saving and restoring .result, I don't
want
> to remove that.

On re-reading them together, I think I agree that at least they're not
redundant.

[Toon Verwaest, 2016-12-05 15:16:36]

Seems like I failed at writing the proper test case and fix the bug :)

lgtm, good catch!

[neis, 2016-12-05 15:42:02]

The CQ bit was checked by neis@chromium.org

[commit-bot: I haz the power, 2016-12-05 15:42:12]

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-12-05 15:47:05]

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

[commit-bot: I haz the power, 2016-12-05 15:47:05]

Try jobs failed on following builders:
  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...)

[neis, 2016-12-05 16:35:21]

The CQ bit was checked by neis@chromium.org

[neis, 2016-12-05 16:35:22]

The patchset sent to the CQ was uploaded after l-g-t-m from adamk@chromium.org,
verwaest@chromium.org
Link to the patchset: https://codereview.chromium.org/2537413003/#ps20001
(title: "Remove unused variable.")

[commit-bot: I haz the power, 2016-12-05 16:35:27]

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-12-05 17:00:03]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1480955721739850,
"parent_rev": "876fd1f51532fab7594b4eace9a395799eb3371a", "commit_rev":
"3d9793bfee8aa934ea08c2933aa05b869a85827b"}

[commit-bot: I haz the power, 2016-12-05 17:00:09]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-12-05 17:00:52]

Description was changed from

==========
[parsing] Fix bug in completion value of try-finally.

R=adamk@chromium.org, verwaest@chromium.org
BUG=v8:5698
==========

to

==========
[parsing] Fix bug in completion value of try-finally.

R=adamk@chromium.org, verwaest@chromium.org
BUG=v8:5698

Committed: https://crrev.com/67f1969a078a30906246e6b6d94d7468de9710f9
Cr-Commit-Position: refs/heads/master@{#41495}
==========

[commit-bot: I haz the power, 2016-12-05 17:00:53]

Patchset 2 (id:??) landed as
https://crrev.com/67f1969a078a30906246e6b6d94d7468de9710f9
Cr-Commit-Position: refs/heads/master@{#41495}