GeneratorFunction() makes generator instances

GeneratorFunction() makes generator instances

The current specification has GeneratorFunction() be like Function(),
except that it makes generator instances.  This commit implements that
behavior.  It also fills in a piece of the implementation where
otherwise calling GeneratorFunction or GeneratorFunctionPrototype would
cause an abort because they have no code.

R=mstarzinger@chromium.org, rossberg@chromium.org
TEST=mjsunit/harmony/generators-iteration
TEST=mjsunit/harmony/generators-runtime
BUG=v8:2355
BUG=v8:2680
Committed: http://code.google.com/p/v8/source/detail?r=14684

[wingo, 2013-05-10 09:29:17]

Please take a look.  This CL depends on
https://codereview.chromium.org/14829005/.

Cheers,

Andy

[rossberg, 2013-05-13 11:33:46]

https://codereview.chromium.org/14857009/diff/1/src/generator.js
File src/generator.js (right):

https://codereview.chromium.org/14857009/diff/1/src/generator.js#newcode76
src/generator.js:76: if (n) {
Nit: n > 0

https://codereview.chromium.org/14857009/diff/1/src/v8natives.js
File src/v8natives.js (right):

https://codereview.chromium.org/14857009/diff/1/src/v8natives.js#newcode1759
src/v8natives.js:1759: // InternalArray -> string -> boolean -> Function
Nit: JS functions aren't curried. ;)

https://codereview.chromium.org/14857009/diff/1/src/v8natives.js#newcode1762
src/v8natives.js:1762: if (formals.length) {
Nit: ... > 0

https://codereview.chromium.org/14857009/diff/1/src/v8natives.js#newcode1768
src/v8natives.js:1768: throw MakeSyntaxError('paren_in_arg_string',[]);
Nit: can you insert a space before []?

https://codereview.chromium.org/14857009/diff/1/src/v8natives.js#newcode1787
src/v8natives.js:1787: if (n) {
Nit: n > 0

But I'd actually avoid the whole copying and simply forward the arguments
object.

[Michael Starzinger, 2013-05-13 11:35:39]

https://codereview.chromium.org/14857009/diff/1/src/v8natives.js
File src/v8natives.js (right):

https://codereview.chromium.org/14857009/diff/1/src/v8natives.js#newcode1785
src/v8natives.js:1785: function NewFunction(arg1) {  // length == 1
Since we are at it, can we rename this to FunctionConstructor, this would be in
line with the rest of the code-base. I have no clue why this one is named
differently. The name is not visible to JavaScript anyways.

[wingo, 2013-05-14 10:11:50]

Fixed nits.  WDYT?  Some comments inline.

https://codereview.chromium.org/14857009/diff/1/src/generator.js
File src/generator.js (right):

https://codereview.chromium.org/14857009/diff/1/src/generator.js#newcode76
src/generator.js:76: if (n) {
On 2013/05/13 11:33:46, rossberg wrote:
> Nit: n > 0

Done.

https://codereview.chromium.org/14857009/diff/1/src/v8natives.js
File src/v8natives.js (right):

https://codereview.chromium.org/14857009/diff/1/src/v8natives.js#newcode1759
src/v8natives.js:1759: // InternalArray -> string -> boolean -> Function
On 2013/05/13 11:33:46, rossberg wrote:
> Nit: JS functions aren't curried. ;)

My notational ignorance showing again :)

https://codereview.chromium.org/14857009/diff/1/src/v8natives.js#newcode1762
src/v8natives.js:1762: if (formals.length) {
On 2013/05/13 11:33:46, rossberg wrote:
> Nit: ... > 0

Done.

https://codereview.chromium.org/14857009/diff/1/src/v8natives.js#newcode1768
src/v8natives.js:1768: throw MakeSyntaxError('paren_in_arg_string',[]);
On 2013/05/13 11:33:46, rossberg wrote:
> Nit: can you insert a space before []?

Done.

https://codereview.chromium.org/14857009/diff/1/src/v8natives.js#newcode1785
src/v8natives.js:1785: function NewFunction(arg1) {  // length == 1
On 2013/05/13 11:35:39, Michael Starzinger wrote:
> Since we are at it, can we rename this to FunctionConstructor, this would be
in
> line with the rest of the code-base. I have no clue why this one is named
> differently. The name is not visible to JavaScript anyways.

Done.  I renamed the nelper to NewFunction, as it seemed more descriptive than
FunctionConstructorHelper; let me know if you prefer another name.

https://codereview.chromium.org/14857009/diff/1/src/v8natives.js#newcode1787
src/v8natives.js:1787: if (n) {
On 2013/05/13 11:33:46, rossberg wrote:
> Nit: n > 0
> 
> But I'd actually avoid the whole copying and simply forward the arguments
> object.

Fixed the nit.  We can't copy arguments as-is because the last item is the
function body, and the formals array is passed to Join().  We could rework that
logic but this function seems a bit fragile; I thought perhaps leaving it as it
was would be best.

[Michael Starzinger, 2013-05-14 13:51:31]

One more comment from my end. Otherwise I would be fine with this change. But
I'll leave the final decision to Andreas.

https://codereview.chromium.org/14857009/diff/6001/src/v8natives.js
File src/v8natives.js (right):

https://codereview.chromium.org/14857009/diff/6001/src/v8natives.js#newcode1787
src/v8natives.js:1787: if (n > 0) {
We should try to have a fast-path for the case where exactly one argument it
passed. Because that is often used as a replacement of eval() in
performance-critical settings and I would like to keep that fast-path without
any allocation of an InternalArray as before. On the other hand I didn't measure
the impact of this change, so if the impact is negligible, then feel free to
ignore my comment.

[rossberg, 2013-05-14 14:05:46]

https://codereview.chromium.org/14857009/diff/1/src/v8natives.js
File src/v8natives.js (right):

https://codereview.chromium.org/14857009/diff/1/src/v8natives.js#newcode1787
src/v8natives.js:1787: if (n) {
On 2013/05/14 10:11:50, wingo wrote:
> On 2013/05/13 11:33:46, rossberg wrote:
> > Nit: n > 0
> > 
> > But I'd actually avoid the whole copying and simply forward the arguments
> > object.
> 
> Fixed the nit.  We can't copy arguments as-is because the last item is the
> function body, and the formals array is passed to Join().  We could rework
that
> logic but this function seems a bit fragile; I thought perhaps leaving it as
it
> was would be best.

Yes, but the code not only copies, but also takes apart the arguments array in
two places, which are completely identical. I see no reason not to push all that
into the helper function.

[wingo, 2013-05-15 08:43:16]

Probably need to handle https://codereview.chromium.org/14668013/ first; Michael
mentioned that he might want to back-port that one to other branches, and it's
probably easier to do the cherry-pick before changing this code.

[wingo, 2013-05-15 12:02:10]

New patch just passes arguments object to helper.  Note that it is rebased. 
PTAL :)

[rossberg, 2013-05-15 12:29:35]

lgtm

[Michael Starzinger, 2013-05-15 12:32:02]

LGTM with a nit. I really hope this doesn't tank performance as mentioned in one
of my comments.

https://codereview.chromium.org/14857009/diff/17002/src/v8natives.js
File src/v8natives.js (right):

https://codereview.chromium.org/14857009/diff/17002/src/v8natives.js#newcode1760
src/v8natives.js:1760: // InternalArray, string, boolean -> Function
nit: The signature is outdated, I think we can just drop it now.

[wingo, 2013-05-15 13:22:14]

Committed patchset #4 manually as r14684 (presubmit successful).