[generators] Add some explanation on forcing context allocation.

[generators] Add some explanation on forcing context allocation.

R=mstarzinger@chromium.org
TBR=rossberg
BUG=

Committed: https://crrev.com/935240f968fc8fe97c8d34ea4c2344738a5403cd
Cr-Commit-Position: refs/heads/master@{#34864}

[Michael Starzinger, 2016-03-17 12:48:00]

LGTM with one suggestion.

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

https://codereview.chromium.org/1806293002/diff/1/src/parsing/parser.cc#newco...
src/parsing/parser.cc:4107: // relies on this forced context allocation, but not
in an essential way.
nit: The "[...], but not in an essential way" part is misleading. Can we just
point to the place where it is relied upon by saying something along the lines
of "[...], because arguments on the stack to the generator are not restored when
resuming a generator" instead. WDYT?

[neis, 2016-03-17 13:39:04]

On 2016/03/17 12:48:00, Michael Starzinger wrote:
> LGTM with one suggestion.
> 
> https://codereview.chromium.org/1806293002/diff/1/src/parsing/parser.cc
> File src/parsing/parser.cc (right):
> 
>
https://codereview.chromium.org/1806293002/diff/1/src/parsing/parser.cc#newco...
> src/parsing/parser.cc:4107: // relies on this forced context allocation, but
not
> in an essential way.
> nit: The "[...], but not in an essential way" part is misleading. Can we just
> point to the place where it is relied upon by saying something along the lines
> of "[...], because arguments on the stack to the generator are not restored
when
> resuming a generator" instead. WDYT?

I don't think that the comment is misleading: the restriction to context
allocation is not essential because it could be easily lifted.  I intentionally
didn't go into details because someone reading the parser code shouldn't need to
know them.  Since it's just one sentence, I don't mind adding the details if you
insist.  Let me know :)

[Michael Starzinger, 2016-03-17 14:19:05]

On 2016/03/17 13:39:04, neis wrote:
> On 2016/03/17 12:48:00, Michael Starzinger wrote:
> > LGTM with one suggestion.
> > 
> > https://codereview.chromium.org/1806293002/diff/1/src/parsing/parser.cc
> > File src/parsing/parser.cc (right):
> > 
> >
>
https://codereview.chromium.org/1806293002/diff/1/src/parsing/parser.cc#newco...
> > src/parsing/parser.cc:4107: // relies on this forced context allocation, but
> not
> > in an essential way.
> > nit: The "[...], but not in an essential way" part is misleading. Can we
just
> > point to the place where it is relied upon by saying something along the
lines
> > of "[...], because arguments on the stack to the generator are not restored
> when
> > resuming a generator" instead. WDYT?
> 
> I don't think that the comment is misleading: the restriction to context
> allocation is not essential because it could be easily lifted.  I
intentionally
> didn't go into details because someone reading the parser code shouldn't need
to
> know them.  Since it's just one sentence, I don't mind adding the details if
you
> insist.  Let me know :)

Acknowledged. Whatever is better for the parser people works for me. It was just
a suggestion. Feel free to land as is. :)

[neis, 2016-03-17 14:20:42]

The CQ bit was checked by neis@chromium.org

[commit-bot: I haz the power, 2016-03-17 14:20:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1806293002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1806293002/1

[commit-bot: I haz the power, 2016-03-17 14:23:38]

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

[commit-bot: I haz the power, 2016-03-17 14:23:39]

Try jobs failed on following builders:
  v8_presubmit on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/12525)

[neis, 2016-03-17 14:24:46]

Description was changed from

==========
[generators] Add some explanation on forcing context allocation.

R=mstarzinger@chromium.org
BUG=
==========

to

==========
[generators] Add some explanation on forcing context allocation.

R=mstarzinger@chromium.org
TBR=rossberg
BUG=
==========

[neis, 2016-03-17 14:24:51]

The CQ bit was checked by neis@chromium.org

[commit-bot: I haz the power, 2016-03-17 14:25:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1806293002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1806293002/1

[commit-bot: I haz the power, 2016-03-17 14:43:36]

Description was changed from

==========
[generators] Add some explanation on forcing context allocation.

R=mstarzinger@chromium.org
TBR=rossberg
BUG=
==========

to

==========
[generators] Add some explanation on forcing context allocation.

R=mstarzinger@chromium.org
TBR=rossberg
BUG=
==========

[commit-bot: I haz the power, 2016-03-17 14:43:37]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-03-17 14:44:22]

Description was changed from

==========
[generators] Add some explanation on forcing context allocation.

R=mstarzinger@chromium.org
TBR=rossberg
BUG=
==========

to

==========
[generators] Add some explanation on forcing context allocation.

R=mstarzinger@chromium.org
TBR=rossberg
BUG=

Committed: https://crrev.com/935240f968fc8fe97c8d34ea4c2344738a5403cd
Cr-Commit-Position: refs/heads/master@{#34864}
==========

[commit-bot: I haz the power, 2016-03-17 14:44:24]

Patchset 1 (id:??) landed as
https://crrev.com/935240f968fc8fe97c8d34ea4c2344738a5403cd
Cr-Commit-Position: refs/heads/master@{#34864}