Move closing of generators upon final return to the generator-resume builtin.

Move closing of generators upon final return to the generator-resume builtin.

Review-Url: https://codereview.chromium.org/2936813002
Cr-Commit-Position: refs/heads/master@{#45906}
Committed: https://chromium.googlesource.com/v8/v8/+/814d08a6af37438efe9f8648a651fd8d0e1f1b10

[Jarin, 2017-06-13 10:01:07]

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

[commit-bot: I haz the power, 2017-06-13 10:01:11]

Dry run: CQ is trying da patch.

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

[Jarin, 2017-06-13 10:01:27]

jarin@chromium.org changed reviewers:
+ neis@chromium.org

[Jarin, 2017-06-13 10:01:41]

Could you take a look, please?

[commit-bot: I haz the power, 2017-06-13 10:32:56]

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

[commit-bot: I haz the power, 2017-06-13 10:32:56]

Dry run: This issue passed the CQ dry run.

[Jarin, 2017-06-13 10:37:01]

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

[commit-bot: I haz the power, 2017-06-13 10:37:04]

Dry run: CQ is trying da patch.

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

[neis, 2017-06-13 10:58:17]

lgtm

https://codereview.chromium.org/2936813002/diff/20001/src/builtins/builtins-g...
File src/builtins/builtins-generator-gen.cc (right):

https://codereview.chromium.org/2936813002/diff/20001/src/builtins/builtins-g...
src/builtins/builtins-generator-gen.cc:29: Node* closed =
SmiConstant(JSGeneratorObject::kGeneratorClosed);
Nit: move this down to where it's used.

https://codereview.chromium.org/2936813002/diff/20001/src/builtins/builtins-g...
src/builtins/builtins-generator-gen.cc:59: // If the generator is not suspended
(i.e., it's state is 'executing'),
s/it's/its/

https://codereview.chromium.org/2936813002/diff/20001/src/builtins/builtins-g...
src/builtins/builtins-generator-gen.cc:71: Return(result);
Please add a comment saying that on suspend the generator function already
wrapped the result.

[commit-bot: I haz the power, 2017-06-13 11:03:17]

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

[commit-bot: I haz the power, 2017-06-13 11:03:17]

Dry run: This issue passed the CQ dry run.

[Jarin, 2017-06-13 11:05:56]

Landing...

https://codereview.chromium.org/2936813002/diff/20001/src/builtins/builtins-g...
File src/builtins/builtins-generator-gen.cc (right):

https://codereview.chromium.org/2936813002/diff/20001/src/builtins/builtins-g...
src/builtins/builtins-generator-gen.cc:29: Node* closed =
SmiConstant(JSGeneratorObject::kGeneratorClosed);
On 2017/06/13 10:58:17, neis wrote:
> Nit: move this down to where it's used.

Done.

https://codereview.chromium.org/2936813002/diff/20001/src/builtins/builtins-g...
src/builtins/builtins-generator-gen.cc:59: // If the generator is not suspended
(i.e., it's state is 'executing'),
On 2017/06/13 10:58:17, neis wrote:
> s/it's/its/

Done.

https://codereview.chromium.org/2936813002/diff/20001/src/builtins/builtins-g...
src/builtins/builtins-generator-gen.cc:71: Return(result);
On 2017/06/13 10:58:17, neis wrote:
> Please add a comment saying that on suspend the generator function already
> wrapped the result.

Done.

[Jarin, 2017-06-13 11:06:00]

The CQ bit was checked by jarin@chromium.org

[Jarin, 2017-06-13 11:06:00]

The patchset sent to the CQ was uploaded after l-g-t-m from neis@chromium.org
Link to the patchset: https://codereview.chromium.org/2936813002/#ps40001
(title: "Address reviewer comments")

[commit-bot: I haz the power, 2017-06-13 11:06:06]

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, 2017-06-13 11:11:18]

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

[commit-bot: I haz the power, 2017-06-13 11:11:19]

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/42963)

[Jarin, 2017-06-13 11:13:42]

jarin@chromium.org changed reviewers:
+ rmcilroy@chromium.org

[Jarin, 2017-06-13 11:13:43]

Ross, could you take a look, please?

[rmcilroy, 2017-06-13 12:36:00]

LGTM, thanks

https://codereview.chromium.org/2936813002/diff/40001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/2936813002/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:2255: // generator-resume builtin.
I'm guessing you could do this for async generators too? Can you add a TODO here
to move closing of async generators too.

[Jarin, 2017-06-13 13:21:02]

The CQ bit was checked by jarin@chromium.org

[Jarin, 2017-06-13 13:21:02]

The patchset sent to the CQ was uploaded after l-g-t-m from neis@chromium.org,
rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/2936813002/#ps60001
(title: "Add TODO")

[commit-bot: I haz the power, 2017-06-13 13:21: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, 2017-06-13 13:49:46]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1497360062034580,
"parent_rev": "322b2d7d14cdf1d6d53fb763e14b9b058f29a020", "commit_rev":
"814d08a6af37438efe9f8648a651fd8d0e1f1b10"}

[commit-bot: I haz the power, 2017-06-13 13:49:52]

Description was changed from

==========
Move closing of generators upon final return to the generator-resume builtin.
==========

to

==========
Move closing of generators upon final return to the generator-resume builtin.

Review-Url: https://codereview.chromium.org/2936813002
Cr-Commit-Position: refs/heads/master@{#45906}
Committed:
https://chromium.googlesource.com/v8/v8/+/814d08a6af37438efe9f8648a651fd8d0e1...
==========

[commit-bot: I haz the power, 2017-06-13 13:49:53]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/v8/v8/+/814d08a6af37438efe9f8648a651fd8d0e1...