Calling a generator function returns a generator object

Calling a generator function returns a generator object

* src/heap.h:
* src/heap.cc:
* src/objects-debug.cc:
* src/objects-inl.h:
* src/objects-printer.cc:
* src/objects-visiting.cc:
* src/objects.cc:
* src/objects.h: Define a new object type, JSGeneratorObject.

* src/factory.h:
* src/factory.cc (NewFunctionFromSharedFunctionInfo): Generator function
  inital maps construct the new JS_GENERATOR_OBJECT_TYPE objects, not
  generic JSObjects.

* src/runtime.h:
* src/runtime.cc (Runtime_CreateJSGeneratorObject):
* src/arm/full-codegen-arm.cc (Generate):
* src/ia32/full-codegen-ia32.cc (Generate):
* src/x64/full-codegen-x64.cc (Generate): Before visiting generator
  bodies, arrange to construct and return a generator object.

* test/mjsunit/harmony/generators-objects.js: Add tests for the
  properties and prototype of generator objects.

BUG=v8:2355
TEST=mjsunit/harmony/generators-objects

Committed: http://code.google.com/p/v8/source/detail?r=14264

[wingo, 2013-04-03 15:37:11]

I ended up calling the "generator object" a "generator iterator", to distinguish
it from "generator functions", and for consistency with the internal naming of
the meta-objects GeneratorFunction and GeneratorIteratorPrototype.

This patch depends on https://codereview.chromium.org/13192004/ and
https://codereview.chromium.org/13408005/.

[wingo, 2013-04-03 16:36:24]

https://codereview.chromium.org/13542002/diff/2001/src/x64/full-codegen-x64.cc
File src/x64/full-codegen-x64.cc (right):

https://codereview.chromium.org/13542002/diff/2001/src/x64/full-codegen-x64.c...
src/x64/full-codegen-x64.cc:287: if (function()->is_generator()) {
How to communicate this iterator object from its allocation to the "yield" call
sites?  I think the answer is to use a local variable.

Instead of fabricating a runtime call here, I think we probably need to add some
nodes to the AST to CallRuntime(CreateJSGeneratorIterator), store it in a
variable, and record that variable in the FunctionState; and follow it with a
runtime call to SuspendGenerator in the prologue, passing the saved generator
iterator as a param.

With forced context allocation, an added local would get put in the context
object, leading to the silliness where the context has a pointer to the iterator
and vice versa.  Oh well; probably something we can live with.

[wingo, 2013-04-03 17:51:00]

Pushed another patch that links generator definitions and uses through a local
variable.  Happily this removes platform-specific code. Now the next step (in
another CL) would be to implement yield and the suspend and resume builtins.

[Michael Starzinger, 2013-04-08 13:14:05]

First round of high-level comment. They are all pretty vague, but the gist of it
is that I think the AST is the wrong level to model these things. Maybe Andreas
has further comments (or different opinions) on that, but I personally would
rather see this done by subsequent compiler phases (as opposed to the parser).

https://codereview.chromium.org/13542002/diff/5001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/13542002/diff/5001/src/objects.cc#newcode1290
src/objects.cc:1290: accumulator->Add("<JS Generator Iterator>");
The debug representation should just be "<JS Generator>" IMHO.

https://codereview.chromium.org/13542002/diff/5001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/13542002/diff/5001/src/objects.h#newcode65
src/objects.h:65: //           - JSGeneratorIterator
As discussed offline, the name "JSGeneratorIterator" is confusing. We should
change it to either "JSGeneratorObject" or "JSGeneratorInstance".

Just for the record, I personally would still name it just "JSGenerator" but I
see that we might want to reserve that name if at some point we need an instance
type for generator functions, so you and Andreas convinced me of the above two
alternatives.

https://codereview.chromium.org/13542002/diff/5001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/13542002/diff/5001/src/parser.cc#newcode1035
src/parser.cc:1035: // For generators, allocate and yield an iterator on
function entry.
As discussed offline, it scares me a little bit to model all of this at the AST
level. I agree that modeling it on some high-level IR would be great, but this
introduces AST nodes that have absolutely no correspondence to the source code.

https://codereview.chromium.org/13542002/diff/5001/src/parser.cc#newcode4422
src/parser.cc:4422: STATIC_ASCII_VECTOR(".generator_iterator"));
The current generator instance needs to be stored in the context, so I totally
see the need for having a context slot. But do we really need a temporary
variable for this and model all of this on the AST level? Can we just assign a
fixed context slot to hold the current generator instance?

https://codereview.chromium.org/13542002/diff/5001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/13542002/diff/5001/src/runtime.cc#newcode1
src/runtime.cc:1: // Copyright 2012, 2013 the V8 project authors. All rights
reserved.
No need to update the copyright header of existing files.

https://codereview.chromium.org/13542002/diff/5001/src/runtime.cc#newcode2394
src/runtime.cc:2394: isolate->factory()->NewJSGeneratorIterator(function);
If we call the Heap::AllocateJSGeneratorIterator directly, we can get by without
a factory wrapper around that.

[wingo, 2013-04-09 07:45:32]

Thanks for the review.  I'm reworking some patches but I would like to make sure
all of the implications of using some other mechanism 

On 2013/04/08 13:14:05, Michael Starzinger wrote:
> https://codereview.chromium.org/13542002/diff/5001/src/parser.cc#newcode1035
> src/parser.cc:1035: // For generators, allocate and yield an iterator on
> function entry.
> As discussed offline, it scares me a little bit to model all of this at the
AST
> level. I agree that modeling it on some high-level IR would be great, but this
> introduces AST nodes that have absolutely no correspondence to the source
code.
> 
> https://codereview.chromium.org/13542002/diff/5001/src/parser.cc#newcode4422
> src/parser.cc:4422: STATIC_ASCII_VECTOR(".generator_iterator"));
> The current generator instance needs to be stored in the context, so I totally
> see the need for having a context slot. But do we really need a temporary
> variable for this and model all of this on the AST level? Can we just assign a
> fixed context slot to hold the current generator instance?

I'm not sure how to reserve context slots by index.  And indeed, the iterator
shouldn't really be in the context anyway because it doesn't need to be saved;
though the temporary approach ends up putting it in the context also through
forced context allocation.

We could reserve a stack slot for the instance, but that seems fragile: one
might want to inline helpers into the generator, or precisely interpret the
meaning of a backtrace, or statically determine the locations of stack handlers
relative to the fp; and it seems brittle to me to allocate slots by number.  The
approach of allocating a temporary variable does not pose problems for
transformation or introspection like this.

We could add a field to FunctionLiteral for the variable that holds the
generator object, but then we'd have to go through contortions to make sure the
variable is actually allocated, as the yield wouldn't explicitly use the
variable.

Dunno.  Apologies for being a bit stubborn here -- you still think reserving a
numbered slot is the right thing, Michael?  Would you do it in the context or on
the stack?

[rossberg, 2013-04-09 09:37:44]

https://codereview.chromium.org/13542002/diff/5001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/13542002/diff/5001/src/parser.cc#newcode1035
src/parser.cc:1035: // For generators, allocate and yield an iterator on
function entry.
On 2013/04/08 13:14:05, Michael Starzinger wrote:
> As discussed offline, it scares me a little bit to model all of this at the
AST
> level. I agree that modeling it on some high-level IR would be great, but this
> introduces AST nodes that have absolutely no correspondence to the source
code.

I think it's perfectly fine. With the more high-level features in ES6+ the AST
will generally move away further from being a direct representation of the
surface syntax. We already do similar things for other constructs, e.g. 'try',
let blocks, and modules, all of which introduce internal statements.

Perhaps the issue is less with the approach as such but with calling the thing
'AST'.

https://codereview.chromium.org/13542002/diff/5001/src/parser.cc#newcode1035
src/parser.cc:1035: // For generators, allocate and yield an iterator on
function entry.
I don't understand, though, why this code is here and not in
ParseFunctionLiteral. That seems odd.

https://codereview.chromium.org/13542002/diff/5001/src/parser.cc#newcode4422
src/parser.cc:4422: STATIC_ASCII_VECTOR(".generator_iterator"));
On 2013/04/08 13:14:05, Michael Starzinger wrote:
> The current generator instance needs to be stored in the context, so I totally
> see the need for having a context slot. But do we really need a temporary
> variable for this and model all of this on the AST level? Can we just assign a
> fixed context slot to hold the current generator instance?

Again, I think this is fine. Hard-coding context slots seems far more hacky and
unmodular to me than introducing a temp var on the syntax level.

[Michael Starzinger, 2013-04-09 10:40:13]

After a longer discussion with Andreas he basically convinced me of the
advantages of modelling generator instantiation in the AST. I was unnecessarily
clinging to the idea that the AST is tightly bound to the source syntax. Calling
the resulting tree AST is misleading though, but that is a different story. So
feel free to ignore two of my comments in that regard.

https://codereview.chromium.org/13542002/diff/5001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/13542002/diff/5001/src/parser.cc#newcode1035
src/parser.cc:1035: // For generators, allocate and yield an iterator on
function entry.
On 2013/04/09 09:37:44, rossberg wrote:
> On 2013/04/08 13:14:05, Michael Starzinger wrote:
> > As discussed offline, it scares me a little bit to model all of this at the
> AST
> > level. I agree that modeling it on some high-level IR would be great, but
this
> > introduces AST nodes that have absolutely no correspondence to the source
> code.
> 
> I think it's perfectly fine. With the more high-level features in ES6+ the AST
> will generally move away further from being a direct representation of the
> surface syntax. We already do similar things for other constructs, e.g. 'try',
> let blocks, and modules, all of which introduce internal statements.
> 
> Perhaps the issue is less with the approach as such but with calling the thing
> 'AST'.

I am convinced, please ignore my initial comment.

https://codereview.chromium.org/13542002/diff/5001/src/parser.cc#newcode4422
src/parser.cc:4422: STATIC_ASCII_VECTOR(".generator_iterator"));
On 2013/04/09 09:37:44, rossberg wrote:
> On 2013/04/08 13:14:05, Michael Starzinger wrote:
> > The current generator instance needs to be stored in the context, so I
totally
> > see the need for having a context slot. But do we really need a temporary
> > variable for this and model all of this on the AST level? Can we just assign
a
> > fixed context slot to hold the current generator instance?
> 
> Again, I think this is fine. Hard-coding context slots seems far more hacky
and
> unmodular to me than introducing a temp var on the syntax level.

I am convinced, please ignore my initial comment.

[wingo, 2013-04-10 10:25:17]

Updated the patch, using the "JSGeneratorObject" name.  See the naming
discussion in https://codereview.chromium.org/13192004/#msg5 for some reasons
why I was hesitant to just call it "JSGenerator".  Is "JSGeneratorInstance" any
better?

Anyway, PTAL.  Thanks :-)

[wingo, 2013-04-11 17:19:42]

Rebased to apply to bleeding_edge, so I guess the side-by-side diffs view is the
right one to use.  If you find this confusing, please delete patch 5 and I will
do something different.

[wingo, 2013-04-11 17:20:12]

On 2013/04/11 17:19:42, wingo wrote:
> Rebased to apply to bleeding_edge, so I guess the side-by-side diffs view is
the
> right one to use.  If you find this confusing, please delete patch 5 and I
will
> do something different.

Patch 6, I mean.

[Michael Starzinger, 2013-04-11 19:17:38]

Looking good already, mostly nits. This should be the last round of comments.

https://codereview.chromium.org/13542002/diff/33001/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/13542002/diff/33001/src/ast.h#newcode2967
src/ast.h:2967: Yield* NewYield(Expression *generator_object, Expression*
expression,
style: Also line-break between first and second argument, so that each argument
is on a separate line.

https://codereview.chromium.org/13542002/diff/33001/src/heap.cc
File src/heap.cc (right):

https://codereview.chromium.org/13542002/diff/33001/src/heap.cc#newcode4348
src/heap.cc:4348: if (!maybe_initial_map->ToObject(&initial_map)) return
maybe_initial_map;
Just use "maybe_initial_map->To()" here and make the initial_map a "Map*", this
avoids casting. I know other places do it in this verbose fashion, but those are
left-overs from a script-driven transformation.

https://codereview.chromium.org/13542002/diff/33001/src/heap.cc#newcode4352
src/heap.cc:4352: ASSERT(map->IsMap());
This assert is obsolete, the accessor should already assert this.

https://codereview.chromium.org/13542002/diff/33001/src/heap.cc#newcode4355
src/heap.cc:4355: MaybeObject* maybe_generator = AllocateJSObjectFromMap(map,
NOT_TENURED);
The NOT_TENURED flag should be the default argument, just skip the argument here
as long this function doesn't support pre-tenuring.

https://codereview.chromium.org/13542002/diff/33001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/13542002/diff/33001/src/parser.cc#newcode1033
src/parser.cc:1033: // For generators, allocate and yield an iterator on
function entry.
Can we move this down into "ParseFunctionLiteral" right before this method is
called? I think it conceptually belongs to the function literal and not to it's
source elements. Also programs/scripts cannot be generators themselves.

https://codereview.chromium.org/13542002/diff/33001/src/parser.h
File src/parser.h (right):

https://codereview.chromium.org/13542002/diff/33001/src/parser.h#newcode555
src/parser.h:555: Variable* generator_object_variable_;
nit: Can we move this up a few lines to the commented variables and add a short
one-liner comment that this is only set for generator function states?

https://codereview.chromium.org/13542002/diff/33001/test/mjsunit/harmony/gene...
File test/mjsunit/harmony/generators-objects.js (right):

https://codereview.chromium.org/13542002/diff/33001/test/mjsunit/harmony/gene...
test/mjsunit/harmony/generators-objects.js:49: 
nit: Add a second empty newline between the two tests.

[wingo, 2013-04-12 10:50:17]

Thanks for the review.  Updated patch fixes nits.

https://codereview.chromium.org/13542002/diff/33001/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/13542002/diff/33001/src/ast.h#newcode2967
src/ast.h:2967: Yield* NewYield(Expression *generator_object, Expression*
expression,
On 2013/04/11 19:17:39, Michael Starzinger wrote:
> style: Also line-break between first and second argument, so that each
argument
> is on a separate line.

Done.

https://codereview.chromium.org/13542002/diff/33001/src/heap.cc
File src/heap.cc (right):

https://codereview.chromium.org/13542002/diff/33001/src/heap.cc#newcode4348
src/heap.cc:4348: if (!maybe_initial_map->ToObject(&initial_map)) return
maybe_initial_map;
On 2013/04/11 19:17:39, Michael Starzinger wrote:
> Just use "maybe_initial_map->To()" here and make the initial_map a "Map*",
this
> avoids casting. I know other places do it in this verbose fashion, but those
are
> left-overs from a script-driven transformation.

Done.

https://codereview.chromium.org/13542002/diff/33001/src/heap.cc#newcode4352
src/heap.cc:4352: ASSERT(map->IsMap());
On 2013/04/11 19:17:39, Michael Starzinger wrote:
> This assert is obsolete, the accessor should already assert this.

Done.

https://codereview.chromium.org/13542002/diff/33001/src/heap.cc#newcode4355
src/heap.cc:4355: MaybeObject* maybe_generator = AllocateJSObjectFromMap(map,
NOT_TENURED);
On 2013/04/11 19:17:39, Michael Starzinger wrote:
> The NOT_TENURED flag should be the default argument, just skip the argument
here
> as long this function doesn't support pre-tenuring.

Done.

https://codereview.chromium.org/13542002/diff/33001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/13542002/diff/33001/src/parser.cc#newcode1033
src/parser.cc:1033: // For generators, allocate and yield an iterator on
function entry.
On 2013/04/11 19:17:39, Michael Starzinger wrote:
> Can we move this down into "ParseFunctionLiteral" right before this method is
> called? I think it conceptually belongs to the function literal and not to
it's
> source elements. Also programs/scripts cannot be generators themselves.

Done.

https://codereview.chromium.org/13542002/diff/33001/src/parser.h
File src/parser.h (right):

https://codereview.chromium.org/13542002/diff/33001/src/parser.h#newcode555
src/parser.h:555: Variable* generator_object_variable_;
On 2013/04/11 19:17:39, Michael Starzinger wrote:
> nit: Can we move this up a few lines to the commented variables and add a
short
> one-liner comment that this is only set for generator function states?

Done.  The comment ended up being a little more verbose.

https://codereview.chromium.org/13542002/diff/33001/test/mjsunit/harmony/gene...
File test/mjsunit/harmony/generators-objects.js (right):

https://codereview.chromium.org/13542002/diff/33001/test/mjsunit/harmony/gene...
test/mjsunit/harmony/generators-objects.js:49: 
On 2013/04/11 19:17:39, Michael Starzinger wrote:
> nit: Add a second empty newline between the two tests.

Done.

[Michael Starzinger, 2013-04-12 11:03:58]

LGTM. I'll land this.

[Michael Starzinger, 2013-04-12 11:32:08]

As discussed offline, there are still test failures caught by the generator
object verification. Will land after this is addressed.

=== mjsunit/harmony/generators-objects ===                    
#
# Fatal error in ../src/objects-debug.cc, line 413
# CHECK(context() == Smi::FromInt(0) || context()->IsContext()) failed
#

[wingo, 2013-04-12 13:07:41]

On 2013/04/12 11:32:08, Michael Starzinger wrote:
> As discussed offline, there are still test failures caught by the generator
> object verification. Will land after this is addressed.
> 
> === mjsunit/harmony/generators-objects ===                    
> #
> # Fatal error in ../src/objects-debug.cc, line 413
> # CHECK(context() == Smi::FromInt(0) || context()->IsContext()) failed
> #

Apologies for letting this one through.  The issue came in tests for "new g()"
-- the caller would use a construct stub to allocate a generator, then the
generator callee allocates its own via a AllocateJSGeneratorObject.  The first
one gets collected but if heap verification runs while it's still alive, the
JSGeneratorObject verifier sees "undefined" as its fields and bails.

Working on a fix, but it will take a little while.

[wingo, 2013-04-12 16:26:02]

New patch fixes generator construction.

There were two bugs.  One was that the "this" value of a "new g()" call was
discarded, whereas it should be valid, bound to "this" during the extent that
the generator object is live, and the same as the generator object that is first
yielded from the function.

The second was that generator objects created by the construct stub, filled in
with undefined, were being verified too zealously.  We relax the verifier to
allow for all fields being undefined, though now that only happens in a short
window.

This patch refactors the allocation and initialization of generator objects to
happen entirely in the runtime, which should detect whether it is in a construct
call or not.

[Michael Starzinger, 2013-04-12 16:53:31]

This last patch is going a little bit off-track. There has to be a simpler
solution without tweaking the initial_map of the JSFunction representing the
generator.

https://codereview.chromium.org/13542002/diff/55001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/13542002/diff/55001/src/runtime.cc#newcode2305
src/runtime.cc:2305: generator = JSGeneratorObject::cast(frame->receiver());
We shouldn't use the implicit receiver as the resulting generator, but allocate
a new one here and return that instead. It will override the implicit receiver.
That's also the solution that we discussed some time ago.

https://codereview.chromium.org/13542002/diff/55001/src/runtime.cc#newcode2308
src/runtime.cc:2308: if (function->has_initial_map()) {
I think we can get by without tweaking the initial map of the generator function
to represent a JSGeneratorObject. It can represent some arbitrary empty
JSObject. Is there another reason why we need to set the initial map of the
generator function at all?

[wingo, 2013-04-12 18:13:46]

On 2013/04/12 16:53:31, Michael Starzinger wrote:
> This last patch is going a little bit off-track. There has to be a simpler
> solution without tweaking the initial_map of the JSFunction representing the
> generator.

That code is not new: it was part of AllocateJSGeneratorObject, now removed. 
It's part of lazy map and prototype allocation.

> https://codereview.chromium.org/13542002/diff/55001/src/runtime.cc
> File src/runtime.cc (right):
> 
> https://codereview.chromium.org/13542002/diff/55001/src/runtime.cc#newcode2305
> src/runtime.cc:2305: generator = JSGeneratorObject::cast(frame->receiver());
> We shouldn't use the implicit receiver as the resulting generator, but
allocate
> a new one here and return that instead. It will override the implicit
receiver.
> That's also the solution that we discussed some time ago.

The problem of that solution is:

  function g() { yield this.foo; }
  var o = new g();
  o.foo = 13
  o.next().value // should evaluate to 13

The implicit receiver for a construct call is already of the right shape for a
generator, and fresh.  Why not use it?

> https://codereview.chromium.org/13542002/diff/55001/src/runtime.cc#newcode2308
> src/runtime.cc:2308: if (function->has_initial_map()) {
> I think we can get by without tweaking the initial map of the generator
function
> to represent a JSGeneratorObject. It can represent some arbitrary empty
> JSObject. Is there another reason why we need to set the initial map of the
> generator function at all?

Here are a couple of reasons:

  g() instanceof g

and

  g.prototype.qux = 10; g().qux // => 10

[Michael Starzinger, 2013-04-14 21:32:50]

You are right, I totally missed the binding of "this" to the generator when a
constructor call is used. I don't want to block this any longer, let's get this
landed. A final round of comments.

https://codereview.chromium.org/13542002/diff/55001/src/objects-debug.cc
File src/objects-debug.cc (right):

https://codereview.chromium.org/13542002/diff/55001/src/objects-debug.cc#newc...
src/objects-debug.cc:414: if (function()->IsUndefined()) {
Just out of curiosity, is there a particular reason we are using Smi(0) as a
sentinel value instead of undefined? I would be fine with switching to undefined
as well. I'll leave that up to you.

https://codereview.chromium.org/13542002/diff/55001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/13542002/diff/55001/src/parser.cc#newcode4350
src/parser.cc:4350: Statement* Parser::BuildGeneratorObjectInitialization() {
No need to have a separate builder for this, please move it back inline as in
the previous patch that I already reviewed.

https://codereview.chromium.org/13542002/diff/55001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/13542002/diff/55001/src/runtime.cc#newcode2305
src/runtime.cc:2305: generator = JSGeneratorObject::cast(frame->receiver());
On 2013/04/12 16:53:31, Michael Starzinger wrote:
> We shouldn't use the implicit receiver as the resulting generator, but
allocate
> a new one here and return that instead. It will override the implicit
receiver.
> That's also the solution that we discussed some time ago.

You are right, I totally missed the fact that the "this" variable will stay
bound to the implicit receiver for the entire lifetime of the generator. Good
point.

https://codereview.chromium.org/13542002/diff/55001/src/runtime.cc#newcode2318
src/runtime.cc:2318: MaybeObject* maybe_generator =
isolate->heap()->Allocate(map, NEW_SPACE);
Please move the JSGenerator allocation back into heap.cc as in the previous
patch that I already reviewed. I am fine with not allocating a new JSGenerator
but reusing the implicit receiver for constructor calls. Not passing the
function to the allocator but leaving it undefined in the allocator is fine. The
values of "function", "context", "continuation" and "operand_stack" can be set
in this runtime function, the allocator can leave them as undefined.

[wingo, 2013-04-15 09:24:56]

Updated patch to address comments.  Apologies for the entropy!

https://codereview.chromium.org/13542002/diff/55001/src/objects-debug.cc
File src/objects-debug.cc (right):

https://codereview.chromium.org/13542002/diff/55001/src/objects-debug.cc#newc...
src/objects-debug.cc:414: if (function()->IsUndefined()) {
On 2013/04/14 21:32:50, Michael Starzinger wrote:
> Just out of curiosity, is there a particular reason we are using Smi(0) as a
> sentinel value instead of undefined? I would be fine with switching to
undefined
> as well. I'll leave that up to you.

Changed to use undefined.  I also relaxed these checks, as it's inconvenient to
type accessors while still allowing for that strange "might-be-undefined" moment
for the Smi and FixedArray fields.

https://codereview.chromium.org/13542002/diff/55001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/13542002/diff/55001/src/parser.cc#newcode4350
src/parser.cc:4350: Statement* Parser::BuildGeneratorObjectInitialization() {
On 2013/04/14 21:32:50, Michael Starzinger wrote:
> No need to have a separate builder for this, please move it back inline as in
> the previous patch that I already reviewed.

Done.

https://codereview.chromium.org/13542002/diff/55001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/13542002/diff/55001/src/runtime.cc#newcode2318
src/runtime.cc:2318: MaybeObject* maybe_generator =
isolate->heap()->Allocate(map, NEW_SPACE);
On 2013/04/14 21:32:50, Michael Starzinger wrote:
> Please move the JSGenerator allocation back into heap.cc as in the previous
> patch that I already reviewed. I am fine with not allocating a new JSGenerator
> but reusing the implicit receiver for constructor calls. Not passing the
> function to the allocator but leaving it undefined in the allocator is fine.
The
> values of "function", "context", "continuation" and "operand_stack" can be set
> in this runtime function, the allocator can leave them as undefined.

Done.

[Michael Starzinger, 2013-04-15 12:04:49]

LGTM again. I'll land this.

[Michael Starzinger, 2013-04-15 12:30:04]

Committed patchset #9 manually as r14264 (presubmit successful).