Generator objects have [[Class]] === "Generator"

Generator objects have [[Class]] === "Generator"

Generator object maps now link to their constructors, which are created
with a "Generator" class name.  This does not cause a per-generator
constructor property to be set.

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

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

[wingo, 2013-04-15 13:36:22]

Attached patch fixes %ClassOf for generator objects.  I had to do it in C++ as
the constructors are per-function, not builtins themselves.

[wingo, 2013-04-16 14:07:35]

Updated patch uses the class name in generator object methods and adds more
tests.

[Michael Starzinger, 2013-04-17 12:52:19]

This is looking almost good. Just few minor comments.

https://codereview.chromium.org/14262004/diff/3001/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/14262004/diff/3001/src/compiler.cc#newcode1126
src/compiler.cc:1126: if (lit->is_generator()) {
Can we move this into Factory::NewSharedFunctionInfo (the one that takes a code
object)? I think that is a more general chocking point for allocation. Also I
would like to keep this a pure copying function without any logic in it. I am
fine with passing the is_generator flag as a simple boolean flag to
NewSharedFunctionInfo for now.

https://codereview.chromium.org/14262004/diff/3001/src/generator.js
File src/generator.js (right):

https://codereview.chromium.org/14262004/diff/3001/src/generator.js#newcode53
src/generator.js:53: ['[Generator].prototype.next', this]);
The error message should say "send" for the method name here.

https://codereview.chromium.org/14262004/diff/3001/src/generator.js#newcode62
src/generator.js:62: ['[Generator].prototype.next', this]);
The error message should say "throw" for the method name here.

https://codereview.chromium.org/14262004/diff/3001/src/generator.js#newcode71
src/generator.js:71: ['[Generator].prototype.next', this]);
The error message should say "close" for the method name here.

https://codereview.chromium.org/14262004/diff/3001/test/mjsunit/harmony/gener...
File test/mjsunit/harmony/generators-objects.js (right):

https://codereview.chromium.org/14262004/diff/3001/test/mjsunit/harmony/gener...
test/mjsunit/harmony/generators-objects.js:58: assertEquals("Generator",
%ClassOf(iter));
Could we also add a check for the ToString representation? The following should
do the trick AFAICT.

assertEquals("[object Generator]", Object.prototype.toString.call(iter));

Or alternatively ...

assertEquals("[object Generator]", String(iter));

https://codereview.chromium.org/14262004/diff/3001/test/mjsunit/harmony/gener...
test/mjsunit/harmony/generators-objects.js:66: assertEquals("Generator",
%ClassOf(iter));
Likewise.

https://codereview.chromium.org/14262004/diff/3001/test/mjsunit/harmony/gener...
test/mjsunit/harmony/generators-objects.js:79: non_generator.next = iter.next;
For primitive values (i.e. the "1" value) this assignment will not cause
iter.next to be called below. Let's rewrite the check to use the following
instead ...

assertThrows("iter.send.call(non_generator)", TypeError);

Likewise for all four functions.

[wingo, 2013-04-17 14:23:51]

Thanks for the review.  Updated patch addresses review comments.

https://codereview.chromium.org/14262004/diff/3001/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/14262004/diff/3001/src/compiler.cc#newcode1126
src/compiler.cc:1126: if (lit->is_generator()) {
On 2013/04/17 12:52:19, Michael Starzinger wrote:
> Can we move this into Factory::NewSharedFunctionInfo (the one that takes a
code
> object)? I think that is a more general chocking point for allocation. Also I
> would like to keep this a pure copying function without any logic in it. I am
> fine with passing the is_generator flag as a simple boolean flag to
> NewSharedFunctionInfo for now.

Done.

https://codereview.chromium.org/14262004/diff/3001/src/generator.js
File src/generator.js (right):

https://codereview.chromium.org/14262004/diff/3001/src/generator.js#newcode53
src/generator.js:53: ['[Generator].prototype.next', this]);
On 2013/04/17 12:52:19, Michael Starzinger wrote:
> The error message should say "send" for the method name here.

Done.

https://codereview.chromium.org/14262004/diff/3001/src/generator.js#newcode62
src/generator.js:62: ['[Generator].prototype.next', this]);
On 2013/04/17 12:52:19, Michael Starzinger wrote:
> The error message should say "throw" for the method name here.

Done.

https://codereview.chromium.org/14262004/diff/3001/src/generator.js#newcode71
src/generator.js:71: ['[Generator].prototype.next', this]);
On 2013/04/17 12:52:19, Michael Starzinger wrote:
> The error message should say "close" for the method name here.

Done.

https://codereview.chromium.org/14262004/diff/3001/test/mjsunit/harmony/gener...
File test/mjsunit/harmony/generators-objects.js (right):

https://codereview.chromium.org/14262004/diff/3001/test/mjsunit/harmony/gener...
test/mjsunit/harmony/generators-objects.js:58: assertEquals("Generator",
%ClassOf(iter));
On 2013/04/17 12:52:19, Michael Starzinger wrote:
> Could we also add a check for the ToString representation? The following
should
> do the trick AFAICT.
> 
> assertEquals("[object Generator]", Object.prototype.toString.call(iter));
> 
> Or alternatively ...
> 
> assertEquals("[object Generator]", String(iter));

Done.

https://codereview.chromium.org/14262004/diff/3001/test/mjsunit/harmony/gener...
test/mjsunit/harmony/generators-objects.js:66: assertEquals("Generator",
%ClassOf(iter));
On 2013/04/17 12:52:19, Michael Starzinger wrote:
> Likewise.

Done.

https://codereview.chromium.org/14262004/diff/3001/test/mjsunit/harmony/gener...
test/mjsunit/harmony/generators-objects.js:79: non_generator.next = iter.next;
On 2013/04/17 12:52:19, Michael Starzinger wrote:
> For primitive values (i.e. the "1" value) this assignment will not cause
> iter.next to be called below. Let's rewrite the check to use the following
> instead ...
> 
> assertThrows("iter.send.call(non_generator)", TypeError);
> 
> Likewise for all four functions.

Done.

[Michael Starzinger, 2013-04-17 14:31:28]

LGTM. I'll land this.

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

https://codereview.chromium.org/14262004/diff/11002/test/mjsunit/harmony/gene...
test/mjsunit/harmony/generators-objects.js:84: non_generator.close = iter.close;
I think the four assignments here are obsolete now.

[Michael Starzinger, 2013-04-17 15:01:43]

Committed patchset #3 manually as r14309 (presubmit successful).