Implement new Function.prototype.toString and fix CreateDynamicFunction parsing

Implement new Function.prototype.toString --harmony-function-tostring

For functions declared in source code, the .toString() representation
will be an excerpt of the source code.
* For functions declared with the "function" keyword, the excerpt
  starts at the "function" or "async" keyword and ends at the final "}".
  The previous behavior would start the excerpt at the "(" of the
  parameter list, and prepend a canonical `"function " + name` or
  similar, which would discard comments and formatting surrounding the
  function's name. Anonymous functions declared as function expressions
  no longer get the name "anonymous" in their toString representation.
* For methods, the excerpt starts at the "get", "set", "*" (for
  generator methods), or property name, whichever comes first.
  Previously, the toString representation for methods would use a
  canonical prefix before the "(" of the parameter list. Note that any
  "static" keyword is omitted.
* For arrow functions and class declarations, the excerpt is unchanged.

For functions created with the Function, GeneratorFunction, or
AsyncFunction constructors:
* The string separating the parameter text and body text is now
  "\n) {\n", where previously it was "\n/*``*/) {\n" or ") {\n".
* At one point, newline normalization was required by the spec here,
  but that was removed from the spec, and so this CL does not do it.

Included in this CL is a fix for CreateDynamicFunction parsing. ')'
and '`' characters in the parameter string are no longer disallowed,
and Function("a=function(", "}){") is no longer allowed.

BUG=v8:4958, v8:4230
Review-Url: https://codereview.chromium.org/2156303002
Cr-Commit-Position: refs/heads/master@{#43262}
Committed: https://chromium.googlesource.com/v8/v8/+/d1d4b9ce51d2ef3e9bd760e70642acddac30846f

[jwolfe, 2016-07-18 18:22:47]

Description was changed from

==========
[WIP] Implement new Function.prototype.toString

Enabled with the flag --harmony-function-tostring

For functions declared in source code, the .toString() representation
will be an excerpt of the source code.
* For functions declared with the "function" keyword, the excerpt
  starts at the "function" keyword and ends at the final "}". The
  previous behavior would start the excerpt at the "(" of the parameter
  list, and prepend a canonical `"function " + name`, which would
  discard comments and formatting surrounding the function's name.
  Anonymous functions declared as function expressions no longer get the
  name "anonymous" in their toString representation.
* For methods, the excerpt starts at the "static", "get", "set", "*"
  (for generator methods), or property name, whichever comes first.
  Previously, the toString representation for methods would start with
  the "(" of the parameter list and completely omit the property name
  and everything else before the "(".
* For arrow functions, the behavior is unchanged.

For functions created with the Function or GeneratorFunction
constructors:
* The function always gets the name "anonymous", where previously it
  would be an unnamed function expression.
* The string separating the parameter text and body text is now
  "\n) {", where previously it was "\n/**/) {\n".

In addition to all of the above:
* Function.prototype.toString always replaces all CRLF and CR with LF
  before returning the string.

TODO: how does this interact with async functions?
TODO: add tests (besides the tests in test262)
TODO: (see TODO(jwolfe) in objects.cc): possibly optimize memory allocations

BUG=v8:4958
LOG=y
==========

to

==========
[WIP] Implement new Function.prototype.toString

Enabled with the flag --harmony-function-tostring

For functions declared in source code, the .toString() representation
will be an excerpt of the source code.
* For functions declared with the "function" keyword, the excerpt
  starts at the "function" keyword and ends at the final "}". The
  previous behavior would start the excerpt at the "(" of the parameter
  list, and prepend a canonical `"function " + name`, which would
  discard comments and formatting surrounding the function's name.
  Anonymous functions declared as function expressions no longer get the
  name "anonymous" in their toString representation.
* For methods, the excerpt starts at the "static", "get", "set", "*"
  (for generator methods), or property name, whichever comes first.
  Previously, the toString representation for methods would start with
  the "(" of the parameter list and completely omit the property name
  and everything else before the "(".
* For arrow functions, the behavior is unchanged.

For functions created with the Function or GeneratorFunction
constructors:
* The function always gets the name "anonymous", where previously it
  would be an unnamed function expression.
* The string separating the parameter text and body text is now
  "\n) {", where previously it was "\n/**/) {\n".

In addition to all of the above:
* Function.prototype.toString always replaces all CRLF and CR with LF
  before returning the string.

TODO: how does this interact with async functions?
TODO: add tests (besides the tests in test262)
TODO: (see TODO(jwolfe) in objects.cc): possibly optimize memory allocations

BUG=v8:4958
LOG=y
==========

[jwolfe, 2016-07-18 18:22:47]

jwolfe@igalia.com changed reviewers:
+ littledan@chromium.org

[jwolfe, 2016-07-18 18:33:10]

Would it be a good idea or a waste of time to add tests to mjsunit/harmony that
test the same thing as the test262 tests?

Is it worthwhile to try to skip the char-by-char copy of the source if there
weren't any CR characters to begin with?

Should the flag be called --harmony-function-prototype-tostring instead of just
--harmony-function-tostring?

Issue 4230 is still a problem. Furthermore, the omission of the /**/ from the
CreateDynamicFunction parsing logic opens the door for even more strange things
to happen.

https://codereview.chromium.org/2156303002/diff/1/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/2156303002/diff/1/src/objects.cc#newcode13030
src/objects.cc:13030: IncrementalStringBuilder& builder) {
Is the use of reference parameters here acceptable? It could easily be rewritten
to use pointers instead.

[Dan Ehrenberg, 2016-07-18 18:52:32]

I don't think we need to include tests which are redundant with test262 tests
beyond a couple basic sanity tests (to make sure this code doesn't completely
rot behind a flag), but it'd be a good idea to take a look at at what's tested
in test262 and if our code paths don't match what's tested there, then add
additional tests (either locally or upstream).

Another important test to run is mjsunit/regress/regress-2470, which is the bug
that introduced the /**/, as explained in the comment on lines 4662-4664. I
think the parser may need a separate entry point for parameter parsing to get
this completely right, unfortunately.

Generally, it'd be a good idea to flip the flag to shipping, run all the tests
(at least mjsunit and cctest, but why not do a fully CQ dry run while you're at
it) and look at what fails. Some things will probably fail as this is a semantic
change, but you may encounter other more interesting failures.

I'm fine with leaving 4230 out of scope for this patch if it doesn't come with
it naturally.

https://codereview.chromium.org/2156303002/diff/1/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/2156303002/diff/1/src/objects.cc#newcode13030
src/objects.cc:13030: IncrementalStringBuilder& builder) {
On 2016/07/18 at 18:33:10, jwolfe wrote:
> Is the use of reference parameters here acceptable? It could easily be
rewritten to use pointers instead.

We tend to use normal pointers in V8 instead.

[jwolfe, 2016-07-18 22:56:41]

Sure enough, mjsunit/regress/regress-2470 fails with this flag enabled. I think
the only real solution to that regression is to fix 4230 properly, such as by
adding as separate entry point for parameter parsing as you suggested. I can
imagine a quicker, dirtier solution than that that involves just running a
scanner on the parameters string and checking for balanced parentheses (and
thereby also checking for unterminated string literals, comments, etc.). I have
medium confidence in that idea.

A total of 15 unique tests are failing in the set "bot_default test262" with
this flag enabled (not counting the ones in the test262 status file referencing
this issue, which all unexpectedly pass now). I expect that I should ignore any
failures that are due to the intended aesthetic changes of this CL. I assume
that updating those tests is a task for another day. I'll keep an eye out for
any legitimate regressions among those 15 failures, such as regress-2470.

[Dan Ehrenberg, 2016-07-19 22:42:48]

On 2016/07/18 at 22:56:41, jwolfe wrote:
> Sure enough, mjsunit/regress/regress-2470 fails with this flag enabled. I
think the only real solution to that regression is to fix 4230 properly, such as
by adding as separate entry point for parameter parsing as you suggested. I can
imagine a quicker, dirtier solution than that that involves just running a
scanner on the parameters string and checking for balanced parentheses (and
thereby also checking for unterminated string literals, comments, etc.). I have
medium confidence in that idea.
> 
> A total of 15 unique tests are failing in the set "bot_default test262" with
this flag enabled (not counting the ones in the test262 status file referencing
this issue, which all unexpectedly pass now). I expect that I should ignore any
failures that are due to the intended aesthetic changes of this CL. I assume
that updating those tests is a task for another day. I'll keep an eye out for
any legitimate regressions among those 15 failures, such as regress-2470.

I'd suggest going with the separate entry point rather than the quick-and-dirty
not-quite parameter error checker. It seems to be that checking for those things
would be pretty difficult to maintain and get correct long-term.

[jwolfe, 2016-07-19 22:44:33]

Description was changed from

==========
[WIP] Implement new Function.prototype.toString

Enabled with the flag --harmony-function-tostring

For functions declared in source code, the .toString() representation
will be an excerpt of the source code.
* For functions declared with the "function" keyword, the excerpt
  starts at the "function" keyword and ends at the final "}". The
  previous behavior would start the excerpt at the "(" of the parameter
  list, and prepend a canonical `"function " + name`, which would
  discard comments and formatting surrounding the function's name.
  Anonymous functions declared as function expressions no longer get the
  name "anonymous" in their toString representation.
* For methods, the excerpt starts at the "static", "get", "set", "*"
  (for generator methods), or property name, whichever comes first.
  Previously, the toString representation for methods would start with
  the "(" of the parameter list and completely omit the property name
  and everything else before the "(".
* For arrow functions, the behavior is unchanged.

For functions created with the Function or GeneratorFunction
constructors:
* The function always gets the name "anonymous", where previously it
  would be an unnamed function expression.
* The string separating the parameter text and body text is now
  "\n) {", where previously it was "\n/**/) {\n".

In addition to all of the above:
* Function.prototype.toString always replaces all CRLF and CR with LF
  before returning the string.

TODO: how does this interact with async functions?
TODO: add tests (besides the tests in test262)
TODO: (see TODO(jwolfe) in objects.cc): possibly optimize memory allocations

BUG=v8:4958
LOG=y
==========

to

==========
[WIP] Implement new Function.prototype.toString

Enabled with the flag --harmony-function-tostring

For functions declared in source code, the .toString() representation
will be an excerpt of the source code.
* For functions declared with the "function" keyword, the excerpt
  starts at the "function" keyword and ends at the final "}". The
  previous behavior would start the excerpt at the "(" of the parameter
  list, and prepend a canonical `"function " + name`, which would
  discard comments and formatting surrounding the function's name.
  Anonymous functions declared as function expressions no longer get the
  name "anonymous" in their toString representation.
* For methods, the excerpt starts at the "get", "set", "*" (for
  generator methods), or property name, whichever comes first.
  Previously, the toString representation for methods would use a
  canonical prefix before the "(" of the parameter list. Note that any
  "static" keyword is omitted.
* For arrow functions and class declarations, the excerpt is unchanged.

For functions created with the Function or GeneratorFunction
constructors:
* The function always gets the name "anonymous", where previously it
  would be an unnamed function expression.
* The string separating the parameter text and body text is now
  "\n) {", where previously it was "\n/**/) {\n".

In addition to all of the above:
* Function.prototype.toString always replaces all CRLF and CR with LF
  before returning the string.

TODO: how does this interact with async functions?
TODO: (see TODO(jwolfe) in objects.cc): possibly optimize memory allocations

BUG=v8:4958
LOG=y
==========

[jwolfe, 2016-07-20 04:14:09]

I added a test for both --harmony-async-await and --harmony-function-tostring
working together. Is that the right thing to do? If async-await was turned on by
default, I would have put those tests in the one test instead of making it
separate. But I'm guessing since the intersection of those two features needs a
unique set of switches, then the tests should be in their own file.

[jwolfe, 2016-07-20 04:14:25]

Description was changed from

==========
[WIP] Implement new Function.prototype.toString

Enabled with the flag --harmony-function-tostring

For functions declared in source code, the .toString() representation
will be an excerpt of the source code.
* For functions declared with the "function" keyword, the excerpt
  starts at the "function" keyword and ends at the final "}". The
  previous behavior would start the excerpt at the "(" of the parameter
  list, and prepend a canonical `"function " + name`, which would
  discard comments and formatting surrounding the function's name.
  Anonymous functions declared as function expressions no longer get the
  name "anonymous" in their toString representation.
* For methods, the excerpt starts at the "get", "set", "*" (for
  generator methods), or property name, whichever comes first.
  Previously, the toString representation for methods would use a
  canonical prefix before the "(" of the parameter list. Note that any
  "static" keyword is omitted.
* For arrow functions and class declarations, the excerpt is unchanged.

For functions created with the Function or GeneratorFunction
constructors:
* The function always gets the name "anonymous", where previously it
  would be an unnamed function expression.
* The string separating the parameter text and body text is now
  "\n) {", where previously it was "\n/**/) {\n".

In addition to all of the above:
* Function.prototype.toString always replaces all CRLF and CR with LF
  before returning the string.

TODO: how does this interact with async functions?
TODO: (see TODO(jwolfe) in objects.cc): possibly optimize memory allocations

BUG=v8:4958
LOG=y
==========

to

==========
[WIP] Implement new Function.prototype.toString

Enabled with the flag --harmony-function-tostring

For functions declared in source code, the .toString() representation
will be an excerpt of the source code.
* For functions declared with the "function" keyword, the excerpt
  starts at the "function" keyword and ends at the final "}". The
  previous behavior would start the excerpt at the "(" of the parameter
  list, and prepend a canonical `"function " + name`, which would
  discard comments and formatting surrounding the function's name.
  Anonymous functions declared as function expressions no longer get the
  name "anonymous" in their toString representation.
* For methods, the excerpt starts at the "get", "set", "*" (for
  generator methods), or property name, whichever comes first.
  Previously, the toString representation for methods would use a
  canonical prefix before the "(" of the parameter list. Note that any
  "static" keyword is omitted.
* For arrow functions and class declarations, the excerpt is unchanged.

For functions created with the Function or GeneratorFunction
constructors:
* The function always gets the name "anonymous", where previously it
  would be an unnamed function expression.
* The string separating the parameter text and body text is now
  "\n) {", where previously it was "\n/**/) {\n".

In addition to all of the above:
* Function.prototype.toString always replaces all CRLF and CR with LF
  before returning the string.

TODO: (see TODO(jwolfe) in objects.cc): possibly optimize memory allocations

BUG=v8:4958
LOG=y
==========

[jwolfe, 2016-07-20 05:44:02]

Results of running all the tests with this flag on:

11 of the tests were simple formatting adjustments, all of which are because the
old .toString() inserts a space after the "function" keyword. i.e.
`function(){}.toString() === "function (){}"`

I'd like to submit a CL where I make whitespace changes to those 11 tests to
make them all pass both with and without --harmony-function-tostring. It was
rather trivial to update them to be oblivious to the flag.

Here are the other 4 failing tests:

mjsunit/regress/regress-2470:
Already discussed above. The way the Function constructor accepts and rejects
parameters is changing slightly. `Function("/*", "*/){")` no longer throws a
SyntaxError like it should.

mjsunit/regress/regress-crbug-109362:
Stack trace line numbers are different due to Function constructor no longer
adding a newline after the "{".

webkit/function-constructor-newline-after-brace:
This test specifically makes sure that a newline is added after the "{", which
it no longer does by design. I suppose this CL makes this test obsolete? There
are some scary comments in the test mentioning "compatibility reasons" without
going into much detail. Not sure what to do about this.

mjsunit/function:
This is a legitimate regression. The [[SourceText]] for the Function constructor
must name the function "anonymous", but when we parse the entire expression
"function anonymous(...) {...}", the name "anonymous" shadows any reference to a
global variable called "anonymous". This bug comes from our strategy to parse
the whole function definition with parameters and body in a single source string
instead of parsing the two separately as the spec calls for (there called "P"
and "bodyText" respectively). I think this also crushes any hope of a quick and
dirty workaround for issue 4230.

[jwolfe, 2016-07-20 06:17:27]

> I'd like to submit a CL where I make whitespace changes to those 11 tests
See: https://codereview.chromium.org/2161413002

[Dan Ehrenberg, 2016-10-26 10:26:45]

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

[commit-bot: I haz the power, 2016-10-26 10:26:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Dan Ehrenberg, 2016-10-26 10:27:25]

How's this patch going? Is it ready for a full review, or is it still an earlier
work in progress?

[commit-bot: I haz the power, 2016-10-26 10:52:16]

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

[commit-bot: I haz the power, 2016-10-26 10:52:17]

Dry run: This issue passed the CQ dry run.

[jwolfe, 2016-10-28 20:09:14]

Description was changed from

==========
[WIP] Implement new Function.prototype.toString

Enabled with the flag --harmony-function-tostring

For functions declared in source code, the .toString() representation
will be an excerpt of the source code.
* For functions declared with the "function" keyword, the excerpt
  starts at the "function" keyword and ends at the final "}". The
  previous behavior would start the excerpt at the "(" of the parameter
  list, and prepend a canonical `"function " + name`, which would
  discard comments and formatting surrounding the function's name.
  Anonymous functions declared as function expressions no longer get the
  name "anonymous" in their toString representation.
* For methods, the excerpt starts at the "get", "set", "*" (for
  generator methods), or property name, whichever comes first.
  Previously, the toString representation for methods would use a
  canonical prefix before the "(" of the parameter list. Note that any
  "static" keyword is omitted.
* For arrow functions and class declarations, the excerpt is unchanged.

For functions created with the Function or GeneratorFunction
constructors:
* The function always gets the name "anonymous", where previously it
  would be an unnamed function expression.
* The string separating the parameter text and body text is now
  "\n) {", where previously it was "\n/**/) {\n".

In addition to all of the above:
* Function.prototype.toString always replaces all CRLF and CR with LF
  before returning the string.

TODO: (see TODO(jwolfe) in objects.cc): possibly optimize memory allocations

BUG=v8:4958
LOG=y
==========

to

==========
[WIP] Implement new Function.prototype.toString

Enabled with the flag --harmony-function-tostring

For functions declared in source code, the .toString() representation
will be an excerpt of the source code.
* For functions declared with the "function" keyword, the excerpt
  starts at the "function" keyword and ends at the final "}". The
  previous behavior would start the excerpt at the "(" of the parameter
  list, and prepend a canonical `"function " + name`, which would
  discard comments and formatting surrounding the function's name.
  Anonymous functions declared as function expressions no longer get the
  name "anonymous" in their toString representation.
* For methods, the excerpt starts at the "get", "set", "*" (for
  generator methods), or property name, whichever comes first.
  Previously, the toString representation for methods would use a
  canonical prefix before the "(" of the parameter list. Note that any
  "static" keyword is omitted.
* For arrow functions and class declarations, the excerpt is unchanged.

For functions created with the Function or GeneratorFunction
constructors:
* The function always gets the name "anonymous", where previously it
  would be an unnamed function expression.
* The string separating the parameter text and body text is now
  "\n) {", where previously it was "\n/**/) {\n".

In addition to all of the above:
* Function.prototype.toString always replaces all CRLF and CR with LF
  before returning the string.

TODO: (see TODO(jwolfe) in objects.cc): possibly optimize memory allocations

BUG=v8:4958, v8:4230
LOG=y
==========

[jwolfe, 2016-10-28 20:13:55]

It looks like there's a viable, straight-forward approach to solving v8:4230 in
this CL.

This CL still needs some work in a few places, like the optimization reparse
from a SharedFunctionInfo.

Also, there are some test failures, that might just be due to the above, or
might be something else.

One area left to research is whether it's possible to get the wrong type of
error if you're supposed to get a SyntaxError for parameter string shenanigans.

[jwolfe, 2016-11-01 21:31:32]

Description was changed from

==========
[WIP] Implement new Function.prototype.toString

Enabled with the flag --harmony-function-tostring

For functions declared in source code, the .toString() representation
will be an excerpt of the source code.
* For functions declared with the "function" keyword, the excerpt
  starts at the "function" keyword and ends at the final "}". The
  previous behavior would start the excerpt at the "(" of the parameter
  list, and prepend a canonical `"function " + name`, which would
  discard comments and formatting surrounding the function's name.
  Anonymous functions declared as function expressions no longer get the
  name "anonymous" in their toString representation.
* For methods, the excerpt starts at the "get", "set", "*" (for
  generator methods), or property name, whichever comes first.
  Previously, the toString representation for methods would use a
  canonical prefix before the "(" of the parameter list. Note that any
  "static" keyword is omitted.
* For arrow functions and class declarations, the excerpt is unchanged.

For functions created with the Function or GeneratorFunction
constructors:
* The function always gets the name "anonymous", where previously it
  would be an unnamed function expression.
* The string separating the parameter text and body text is now
  "\n) {", where previously it was "\n/**/) {\n".

In addition to all of the above:
* Function.prototype.toString always replaces all CRLF and CR with LF
  before returning the string.

TODO: (see TODO(jwolfe) in objects.cc): possibly optimize memory allocations

BUG=v8:4958, v8:4230
LOG=y
==========

to

==========
Implement new Function.prototype.toString --harmony-function-tostring

For functions declared in source code, the .toString() representation
will be an excerpt of the source code.
* For functions declared with the "function" keyword, the excerpt
  starts at the "function" keyword and ends at the final "}". The
  previous behavior would start the excerpt at the "(" of the parameter
  list, and prepend a canonical `"function " + name`, which would
  discard comments and formatting surrounding the function's name.
  Anonymous functions declared as function expressions no longer get the
  name "anonymous" in their toString representation.
* For methods, the excerpt starts at the "get", "set", "*" (for
  generator methods), or property name, whichever comes first.
  Previously, the toString representation for methods would use a
  canonical prefix before the "(" of the parameter list. Note that any
  "static" keyword is omitted.
* For arrow functions and class declarations, the excerpt is unchanged.

For functions created with the Function, GeneratorFunction, or AsynccFunction
constructors:
* The string separating the parameter text and body text is now
  "\n) {", where previously it was "\n/**/) {\n" or ") {\n".

In addition to all of the above:
* Function.prototype.toString always replaces all CRLF and CR with LF
  before returning the string.

Included in this CL is a fix for CreateDynamicFunction parsing. ')'
characters in the parameter string are no longer disallowed, and
Function("a=function(", "}){") is no longer allowed.

BUG=v8:4958, v8:4230
==========

[jwolfe, 2016-11-01 21:34:19]

PTAL. I've still got some test work to do, but I believe the code here is
complete.

https://codereview.chromium.org/2156303002/diff/100001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/2156303002/diff/100001/src/objects.cc#newcode...
src/objects.cc:13712: // to specify it?
this is an optional optimization we may want to do.

[caitp, 2016-11-02 02:41:32]

On 2016/07/18 18:52:32, Dan Ehrenberg wrote:
> I don't think we need to include tests which are redundant with test262 tests
> beyond a couple basic sanity tests (to make sure this code doesn't completely
> rot behind a flag), but it'd be a good idea to take a look at at what's tested
> in test262 and if our code paths don't match what's tested there, then add
> additional tests (either locally or upstream).
> 
> Another important test to run is mjsunit/regress/regress-2470, which is the
bug
> that introduced the /**/, as explained in the comment on lines 4662-4664. I
> think the parser may need a separate entry point for parameter parsing to get
> this completely right, unfortunately.

I've looked at this in JSC, and I think doing it this way is the more sensible
way to
solve the various screw-ups that are possible. I don't think it should be much
of
a problem, though (eg, the separate entry point doesn't need to be exposed to
the API or anything).

> Generally, it'd be a good idea to flip the flag to shipping, run all the tests
> (at least mjsunit and cctest, but why not do a fully CQ dry run while you're
at
> it) and look at what fails. Some things will probably fail as this is a
semantic
> change, but you may encounter other more interesting failures.

I'm not sure I agree with putting this behind a flag at all. Virtually nothing
changes
with this except for fewer restrictions on the kinds of code that can be parsed,
such
as allowing the single-line comment in formal parameters, and not restricting
parameter
expressions just for containing parentheses.

Other than lifting these restrictions, the only real breaking point is the
slightly different
output from F.p.toString(), right? That itself has changed in the not too
distant past,
and if anybody was broken by it, they seem to have dealt with it pretty quietly,
far as
I can tell.

> I'm fine with leaving 4230 out of scope for this patch if it doesn't come with
> it naturally.
> 
> https://codereview.chromium.org/2156303002/diff/1/src/objects.cc
> File src/objects.cc (right):
> 
> https://codereview.chromium.org/2156303002/diff/1/src/objects.cc#newcode13030
> src/objects.cc:13030: IncrementalStringBuilder& builder) {
> On 2016/07/18 at 18:33:10, jwolfe wrote:
> > Is the use of reference parameters here acceptable? It could easily be
> rewritten to use pointers instead.
> 
> We tend to use normal pointers in V8 instead.

[jwolfe, 2016-12-01 18:52:54]

The following test fails with --harmony-funtion-tostring:
mjsunit/regress/regress-2470

This is due to this code right here:

// Some embedders rely on the string representation of the resulting
// function in cases where no formal parameters are specified.
var asString = Function("return 23").toString();
assertSame("function anonymous() {\nreturn 23\n}", asString);

The spec requires a \n in the parameter list even when there are no parameters,
so this will necessarily regress in the way the comment says. What should I do
about this test in this patch?

Here's another test that might cause trouble:
mjsunit/regress/regress-crbug-109362

That test checks for the line:column info for dynamic functions. Line:column
info must necessarily change due to no longer inserting a \n at the beginning of
the body text.

[Dan Ehrenberg, 2016-12-01 18:54:32]

On 2016/12/01 18:52:54, jwolfe wrote:
> The following test fails with --harmony-funtion-tostring:
> mjsunit/regress/regress-2470
> 
> This is due to this code right here:
> 
> // Some embedders rely on the string representation of the resulting
> // function in cases where no formal parameters are specified.
> var asString = Function("return 23").toString();
> assertSame("function anonymous() {\nreturn 23\n}", asString);
> 
> The spec requires a \n in the parameter list even when there are no
parameters,
> so this will necessarily regress in the way the comment says. What should I do
> about this test in this patch?
> 
> Here's another test that might cause trouble:
> mjsunit/regress/regress-crbug-109362
> 
> That test checks for the line:column info for dynamic functions. Line:column
> info must necessarily change due to no longer inserting a \n at the beginning
of
> the body text.

Yes, the new specification causes this observable change, so the tests should be
changed as well. You can change the test as part of the follow-on patch that
turns on the flag.

[jwolfe, 2016-12-02 00:49:56]

Description was changed from

==========
Implement new Function.prototype.toString --harmony-function-tostring

For functions declared in source code, the .toString() representation
will be an excerpt of the source code.
* For functions declared with the "function" keyword, the excerpt
  starts at the "function" keyword and ends at the final "}". The
  previous behavior would start the excerpt at the "(" of the parameter
  list, and prepend a canonical `"function " + name`, which would
  discard comments and formatting surrounding the function's name.
  Anonymous functions declared as function expressions no longer get the
  name "anonymous" in their toString representation.
* For methods, the excerpt starts at the "get", "set", "*" (for
  generator methods), or property name, whichever comes first.
  Previously, the toString representation for methods would use a
  canonical prefix before the "(" of the parameter list. Note that any
  "static" keyword is omitted.
* For arrow functions and class declarations, the excerpt is unchanged.

For functions created with the Function, GeneratorFunction, or AsynccFunction
constructors:
* The string separating the parameter text and body text is now
  "\n) {", where previously it was "\n/**/) {\n" or ") {\n".

In addition to all of the above:
* Function.prototype.toString always replaces all CRLF and CR with LF
  before returning the string.

Included in this CL is a fix for CreateDynamicFunction parsing. ')'
characters in the parameter string are no longer disallowed, and
Function("a=function(", "}){") is no longer allowed.

BUG=v8:4958, v8:4230
==========

to

==========
Implement new Function.prototype.toString --harmony-function-tostring

For functions declared in source code, the .toString() representation
will be an excerpt of the source code.
* For functions declared with the "function" keyword, the excerpt
  starts at the "function" keyword and ends at the final "}". The
  previous behavior would start the excerpt at the "(" of the parameter
  list, and prepend a canonical `"function " + name`, which would
  discard comments and formatting surrounding the function's name.
  Anonymous functions declared as function expressions no longer get the
  name "anonymous" in their toString representation.
* For methods, the excerpt starts at the "get", "set", "*" (for
  generator methods), or property name, whichever comes first.
  Previously, the toString representation for methods would use a
  canonical prefix before the "(" of the parameter list. Note that any
  "static" keyword is omitted.
* For arrow functions and class declarations, the excerpt is unchanged.

For functions created with the Function, GeneratorFunction, or AsynccFunction
constructors:
* The string separating the parameter text and body text is now
  "\n) {", where previously it was "\n/**/) {\n" or ") {\n".

Note that this CL does not do newline normalization.

Included in this CL is a fix for CreateDynamicFunction parsing. ')'
characters in the parameter string are no longer disallowed, and
Function("a=function(", "}){") is no longer allowed.

BUG=v8:4958, v8:4230
==========

[jwolfe, 2016-12-02 00:51:25]

PTAL.

The following microbenchmark demonstrates no performance regression in
Function(), and a significant performance improvement in
Function.prototype.toString(). The improvement is probably due to slicing
sourcecode instead of building a string. Note that this CL does not do newline
normalization in Function.prototype.toString().


var time;
var longParameterString = "a";
for (var i = 0; i < 1000; i++) {
  longParameterString += ", a" + i;
}

time = new Date().getTime();
for (var i = 0; i < 1000000; i++) {
  Function(i.toString());
}
print("just Function(): " + (new Date().getTime() - time));

var f = Function("a", "b", "c", "return a + b + c;");
time = new Date().getTime();
for (var i = 0; i < 100000000; i++) {
  f.toString();
}
print("just toString(): " + (new Date().getTime() - time));

time = new Date().getTime();
for (var i = 0; i < 1000000; i++) {
  Function(i.toString()).toString();
}
print("simple: " + (new Date().getTime() - time));

time = new Date().getTime();
for (var i = 0; i < 50000; i++) {
  Function(longParameterString, i.toString()).toString();
}
print("long parameters: " + (new Date().getTime() - time));


Results:

master$ for f in 1 2 3; do ./out/x64.release/d8 --allow-natives-syntax
test/mjsunit/mjsunit.js test.sln; done
just Function(): 15962
just toString(): 20245
simple: 16216
long parameters: 30636
just Function(): 15618
just toString(): 21166
simple: 16239
long parameters: 31639
just Function(): 15921
just toString(): 21218
simple: 16152
long parameters: 30732

this-CL$ for f in 1 2 3; do ./out/x64.release/d8 --allow-natives-syntax
--harmony-function-tostring test/mjsunit/mjsunit.js test.sln; done
just Function(): 15883
just toString(): 6788
simple: 16768
long parameters: 30762
just Function(): 16420
just toString(): 6465
simple: 16039
long parameters: 30142
just Function(): 16294
just toString(): 7048
simple: 16190
long parameters: 30620

[Dan Ehrenberg, 2016-12-06 00:32:13]

I really like this implementation strategy, and it's great news that
implementing the new semantics gave a speed improvement! It would be nice to see
if this affects the performance of the Speedometer Angular test.

https://codereview.chromium.org/2156303002/diff/140001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/2156303002/diff/140001/src/api.cc#newcode2265
src/api.cc:2265: // TODO(jwolfe): restrict parameters_end_pos?
Could you elaborate a bit more in this TODO? Why would you restrict it, and what
effect would that have?

https://codereview.chromium.org/2156303002/diff/140001/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/2156303002/diff/140001/src/compiler.cc#newcod...
src/compiler.cc:1291: eval_scope_position = -parameters_end_pos;
This seems like a valid implementation strategy, taking advantage of the way
that the cache stores a tuple in a nice way to reduce code duplication. However,
it's a little confusing to a casual reader which might not start looking through
the code from this well-written comment. The only other comment I found that
documents what eval_scope_position is for was in
CompilationCacheTable::LookupEval in src/objects.cc; at least that comment
should be updated. It may also be a good idea to change the name of this
parameter ("position"? or something generic to indicate that this is an
otherwise-uninterpreted token? I am not sure what would be best) to indicate its
multiple uses.

https://codereview.chromium.org/2156303002/diff/140001/src/messages.h
File src/messages.h (right):

https://codereview.chromium.org/2156303002/diff/140001/src/messages.h#newcode585
src/messages.h:585: T(ExpectedEndOfParameters, "Expected arg string to end
here")                \
Aren't we talking about cases where someone effectively uses invalid syntax in
parameters for the function constructor? The exception text could reflect that
more directly.

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

https://codereview.chromium.org/2156303002/diff/140001/src/parsing/parser.cc#...
src/parsing/parser.cc:3172: // This is the first function encountered in an
only-single-function eval.
I thought this happens only in the Function constructor

https://codereview.chromium.org/2156303002/diff/140001/src/parsing/parser.cc#...
src/parsing/parser.cc:3186: MessageTemplate::kExpectedEndOfParameters);
Will CreateDynamicFunction always trigger eager compilation? If the function
constructor sometimes does lazy compilation, then this logic should be
implemented in the preparser as well. Otherwise, could we add a DCHECK in
Parser::SkipFunction to ensure that parameters_end_pos_ == kNoSourcePosition to
double-check that this never occurs?

https://codereview.chromium.org/2156303002/diff/140001/src/parsing/parser.h
File src/parsing/parser.h (right):

https://codereview.chromium.org/2156303002/diff/140001/src/parsing/parser.h#n...
src/parsing/parser.h:1129: V8_INLINE bool IgnoreFunctionName() const {
Naming nit: How about something that's more about what this signifies, rather
than what it's used for? Such as, IsDynamicFunction()

[jwolfe, 2017-01-13 00:28:48]

https://codereview.chromium.org/2156303002/diff/140001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/2156303002/diff/140001/src/api.cc#newcode2265
src/api.cc:2265: // TODO(jwolfe): restrict parameters_end_pos?
On 2016/12/06 00:32:13, Dan Ehrenberg wrote:
> Could you elaborate a bit more in this TODO? Why would you restrict it, and
what
> effect would that have?

I just did it, and it seems like the right thing to do.

What this means is that mismatched parentheses or backticks will cause a syntax
error through this API just like they do for CreateDynamicFunction().

https://codereview.chromium.org/2156303002/diff/140001/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/2156303002/diff/140001/src/compiler.cc#newcod...
src/compiler.cc:1291: eval_scope_position = -parameters_end_pos;
On 2016/12/06 00:32:13, Dan Ehrenberg wrote:
> a casual reader which might not start looking through
> the code from this well-written comment.
I added documentation to the StringSharedKey constructor and removed the comment
in objects.cc.

> It may also be a good idea to change the name of this
> parameter ("position"? or something generic to indicate that this is an
> otherwise-uninterpreted token? I am not sure what would be best) to indicate
its
> multiple uses.
I have not done this yet. I'll rebase before renaming anything.

https://codereview.chromium.org/2156303002/diff/140001/src/messages.h
File src/messages.h (right):

https://codereview.chromium.org/2156303002/diff/140001/src/messages.h#newcode585
src/messages.h:585: T(ExpectedEndOfParameters, "Expected arg string to end
here")                \
On 2016/12/06 00:32:13, Dan Ehrenberg wrote:
> Aren't we talking about cases where someone effectively uses invalid syntax in
> parameters for the function constructor? The exception text could reflect that
> more directly.

I split the error into two different messages:

(function anonymous() { function f(
                    ^
SyntaxError: Arg string terminates parameters early

and:

(function anonymous(a=function(
                              ^^
SyntaxError: Unexpected end of arg string

I'm open to feedback on making these errors more clear.

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

https://codereview.chromium.org/2156303002/diff/140001/src/parsing/parser.cc#...
src/parsing/parser.cc:3172: // This is the first function encountered in an
only-single-function eval.
On 2016/12/06 00:32:13, Dan Ehrenberg wrote:
> I thought this happens only in the Function constructor

Updated comment to be more clear.

https://codereview.chromium.org/2156303002/diff/140001/src/parsing/parser.cc#...
src/parsing/parser.cc:3186: MessageTemplate::kExpectedEndOfParameters);
On 2016/12/06 00:32:13, Dan Ehrenberg wrote:
> Will CreateDynamicFunction always trigger eager compilation? If the function
> constructor sometimes does lazy compilation, then this logic should be
> implemented in the preparser as well. Otherwise, could we add a DCHECK in
> Parser::SkipFunction to ensure that parameters_end_pos_ == kNoSourcePosition
to
> double-check that this never occurs?

Done. It looks like it's always eagerly parsed. I have a little difficulty
tracing the eager vs lazy logic, but it looked to me like it was going to be
always eager. I added the DCHECK also, and it didn't trigger anywhere in
bot_default+test262 (I made it a CHECK for that test).

https://codereview.chromium.org/2156303002/diff/140001/src/parsing/parser.h
File src/parsing/parser.h (right):

https://codereview.chromium.org/2156303002/diff/140001/src/parsing/parser.h#n...
src/parsing/parser.h:1129: V8_INLINE bool IgnoreFunctionName() const {
On 2016/12/06 00:32:13, Dan Ehrenberg wrote:
> Naming nit: How about something that's more about what this signifies, rather
> than what it's used for? Such as, IsDynamicFunction()

How about ParsingDynamicFunctionDeclaration? The semantics are a bit strange,
because the return value becomes false after the parameters are done being
parsed. This is so that it doesn't affect any functions declared inside the
dynamic function. So I think some extra words in the name are justified to match
the non-trivial behavior, but I know I disagree on this kind of thing with
people quite often, so I'm still open to other naming ideas.

[jwolfe, 2017-01-13 22:53:42]

PTAL.

I used the name "position" instead of "scope_position" per your suggestion.

[Dan Ehrenberg, 2017-01-14 00:04:44]

Could you enable test262 tests that this fixes? See
https://docs.google.com/document/d/16bj7AIDgZLv4WOsUEzQ5NzcEN9_xo095e88Pz8FC5...
for context. (Sorry, this was added after you started on the patch. But you'll
get more test coverage for free!)

https://codereview.chromium.org/2156303002/diff/200001/src/parsing/parser.h
File src/parsing/parser.h (right):

https://codereview.chromium.org/2156303002/diff/200001/src/parsing/parser.h#n...
src/parsing/parser.h:1113: V8_INLINE bool ParsingDynamicFunctionDeclaration()
const {
Nit: How about IsParsingDynamicFunctionDeclaration? There's a general pattern of
functions/methods that return a boolean starting with Is.

[Dan Ehrenberg, 2017-01-14 00:04:47]

Could you enable test262 tests that this fixes? See
https://docs.google.com/document/d/16bj7AIDgZLv4WOsUEzQ5NzcEN9_xo095e88Pz8FC5...
for context. (Sorry, this was added after you started on the patch. But you'll
get more test coverage for free!)

[jwolfe, 2017-02-04 00:48:11]

PTAL.

On 2017/01/14 00:04:47, Dan Ehrenberg wrote:
> Could you enable test262 tests that this fixes?

done. Here are the outstanding FAIL tests in test262.status for this feature:

built-ins/Function/prototype/toString/AsyncFunction
- test is broken. see https://github.com/tc39/test262/pull/847

built-ins/Function/prototype/toString/line-terminator-normalisation-CR
built-ins/Function/prototype/toString/line-terminator-normalisation-CR-LF
- tests have been updated. We just need a DEPS update.

[Dan Ehrenberg, 2017-02-05 09:38:08]

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

[commit-bot: I haz the power, 2017-02-05 09:38:16]

Dry run: 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-02-05 10:00:26]

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

[commit-bot: I haz the power, 2017-02-05 10:00:27]

Dry run: Try jobs failed on following builders:
  v8_win64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/22054)
  v8_win64_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng_triggered/b...)
  v8_win_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/22144)
  v8_win_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng_triggered/bui...)

[Dan Ehrenberg, 2017-02-05 10:19:38]

The CQ seems to show a crash; do you know what this is?

Thanks for going through the current causes of the failures. I like to include
those in the test262.status file as well, to help address them later. They could
go in the section for incorrect tests.

[jwolfe, 2017-02-07 18:50:23]

The CQ bit was checked by jwolfe@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-07 18:50:31]

Dry run: 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-02-07 18:50:32]

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

[commit-bot: I haz the power, 2017-02-07 18:50:33]

Dry run: No L-G-T-M from a valid reviewer yet. 
CQ run can only be started by full committers or once the patch has
received an L-G-T-M from a full committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
Committers are members of the group "project-v8-committers".
Note that this has nothing to do with OWNERS files.

[Dan Ehrenberg, 2017-02-11 07:39:11]

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

[commit-bot: I haz the power, 2017-02-11 07:39:13]

Dry run: 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-02-11 08:39:53]

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

[commit-bot: I haz the power, 2017-02-11 08:39:54]

Dry run: This issue passed the CQ dry run.

[Dan Ehrenberg, 2017-02-13 17:32:32]

https://codereview.chromium.org/2156303002/diff/260001/src/parsing/parser-base.h
File src/parsing/parser-base.h (right):

https://codereview.chromium.org/2156303002/diff/260001/src/parsing/parser-bas...
src/parsing/parser-base.h:3431: Consume(Token::IDENTIFIER);
Optional: You could put a DCHECK here that it actually is anonymous (and the
same below). I agree that it always should be, though.

But why are we special-casing this path?

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

https://codereview.chromium.org/2156303002/diff/260001/src/parsing/parser.cc#...
src/parsing/parser.cc:3147: } else if (position > expected_parameters_end_pos) {
Optional: This branch will always be taken if we reach this line. It'd be nicer
to have a DCHECK to assert that it's true rather than a test.

[jwolfe, 2017-02-13 21:31:05]

https://codereview.chromium.org/2156303002/diff/260001/src/parsing/parser-base.h
File src/parsing/parser-base.h (right):

https://codereview.chromium.org/2156303002/diff/260001/src/parsing/parser-bas...
src/parsing/parser-base.h:3431: Consume(Token::IDENTIFIER);
On 2017/02/13 17:32:32, Dan Ehrenberg wrote:
> Optional: You could put a DCHECK here that it actually is anonymous (and the
> same below). I agree that it always should be, though.
Done.

> But why are we special-casing this path?
To make sure the function does not get a name. We need the name `anonymous` from
inside the function body to bind to any `anonymous` identifier declared outside
the function, not to the function itself. And the reason the name `anonymous` is
in the source string at all is because it's required by toString().

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

https://codereview.chromium.org/2156303002/diff/260001/src/parsing/parser.cc#...
src/parsing/parser.cc:3147: } else if (position > expected_parameters_end_pos) {
On 2017/02/13 17:32:32, Dan Ehrenberg wrote:
> Optional: This branch will always be taken if we reach this line. It'd be
nicer
> to have a DCHECK to assert that it's true rather than a test.

The case where position == expected_parameters_end_pos is the success case, so
all three branches are possible here. Perhaps it would be more optimal to first
check if the two are equal to so that we do only 1 check in the common case.
That would look like this:

if (position != expected_parameters_end_pos) {
  // error
  if (position > expected_parameters_end_pos) {
  } else {
  }
}
// success

Are optimizations that subtle something we should be worrying about?

[Dan Ehrenberg, 2017-02-14 12:06:00]

lgtm

Great work, I think this is good to commit.

https://codereview.chromium.org/2156303002/diff/280001/src/parsing/parser-base.h
File src/parsing/parser-base.h (right):

https://codereview.chromium.org/2156303002/diff/280001/src/parsing/parser-bas...
src/parsing/parser-base.h:3430: // "anonymous".
Optional: Instead of saying what this is doing (which is sort of apparent from
the next line), the comment could explain that it's doing this to avoid giving
the name "anonymous", because it's synthetic.

[jwolfe, 2017-02-14 20:16:39]

The CQ bit was checked by jwolfe@igalia.com

[jwolfe, 2017-02-14 20:16:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
littledan@chromium.org
Link to the patchset: https://codereview.chromium.org/2156303002/#ps300001
(title: "update comment for removing anonymous identifier")

[commit-bot: I haz the power, 2017-02-14 20:16:49]

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-02-14 20:19:58]

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

[commit-bot: I haz the power, 2017-02-14 20:20:00]

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

[jwolfe, 2017-02-14 20:31:46]

Description was changed from

==========
Implement new Function.prototype.toString --harmony-function-tostring

For functions declared in source code, the .toString() representation
will be an excerpt of the source code.
* For functions declared with the "function" keyword, the excerpt
  starts at the "function" keyword and ends at the final "}". The
  previous behavior would start the excerpt at the "(" of the parameter
  list, and prepend a canonical `"function " + name`, which would
  discard comments and formatting surrounding the function's name.
  Anonymous functions declared as function expressions no longer get the
  name "anonymous" in their toString representation.
* For methods, the excerpt starts at the "get", "set", "*" (for
  generator methods), or property name, whichever comes first.
  Previously, the toString representation for methods would use a
  canonical prefix before the "(" of the parameter list. Note that any
  "static" keyword is omitted.
* For arrow functions and class declarations, the excerpt is unchanged.

For functions created with the Function, GeneratorFunction, or AsynccFunction
constructors:
* The string separating the parameter text and body text is now
  "\n) {", where previously it was "\n/**/) {\n" or ") {\n".

Note that this CL does not do newline normalization.

Included in this CL is a fix for CreateDynamicFunction parsing. ')'
characters in the parameter string are no longer disallowed, and
Function("a=function(", "}){") is no longer allowed.

BUG=v8:4958, v8:4230
==========

to

==========
Implement new Function.prototype.toString --harmony-function-tostring

For functions declared in source code, the .toString() representation
will be an excerpt of the source code.
* For functions declared with the "function" keyword, the excerpt
  starts at the "function" or "async" keyword and ends at the final "}".
  The previous behavior would start the excerpt at the "(" of the
  parameter list, and prepend a canonical `"function " + name` or
  similar, which would discard comments and formatting surrounding the
  function's name. Anonymous functions declared as function expressions
  no longer get the name "anonymous" in their toString representation.
* For methods, the excerpt starts at the "get", "set", "*" (for
  generator methods), or property name, whichever comes first.
  Previously, the toString representation for methods would use a
  canonical prefix before the "(" of the parameter list. Note that any
  "static" keyword is omitted.
* For arrow functions and class declarations, the excerpt is unchanged.

For functions created with the Function, GeneratorFunction, or
AsyncFunction constructors:
* The string separating the parameter text and body text is now
  "\n) {\n", where previously it was "\n/*``*/) {\n" or ") {\n".
* At one point, newline normalization was required by the spec here,
  but that was removed from the spec, and so this CL does not do it.

Included in this CL is a fix for CreateDynamicFunction parsing. ')'
and '`' characters in the parameter string are no longer disallowed,
and Function("a=function(", "}){") is no longer allowed.

BUG=v8:4958, v8:4230
==========

[Dan Ehrenberg, 2017-02-14 20:44:42]

littledan@chromium.org changed reviewers:
+ yangguo@chromium.org

[Dan Ehrenberg, 2017-02-14 20:44:43]

Adding Yang as a debug owner

[Yang, 2017-02-16 14:14:38]

On 2017/02/14 20:44:43, Dan Ehrenberg wrote:
> Adding Yang as a debug owner

src/debug lgtm.

[Dan Ehrenberg, 2017-02-16 14:36:47]

The CQ bit was checked by littledan@chromium.org

[commit-bot: I haz the power, 2017-02-16 14:36:56]

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-02-16 15:07:22]

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

[commit-bot: I haz the power, 2017-02-16 15:07:25]

Failed to apply patch for src/bootstrapper.cc:
While running git apply --index -p1;
  error: patch failed: src/bootstrapper.cc:3532
  error: src/bootstrapper.cc: patch does not apply

Patch:       src/bootstrapper.cc
Index: src/bootstrapper.cc
diff --git a/src/bootstrapper.cc b/src/bootstrapper.cc
index
dbc7f928e8c6a0958b78f9f6bf3add886d6be14a..dda24fe9e37f1c5dfcace806b8a36abca1e47577
100644
--- a/src/bootstrapper.cc
+++ b/src/bootstrapper.cc
@@ -3532,6 +3532,7 @@ EMPTY_INITIALIZE_GLOBAL_FOR_FEATURE(icu_case_mapping)
 #endif
 EMPTY_INITIALIZE_GLOBAL_FOR_FEATURE(harmony_restrictive_generators)
 EMPTY_INITIALIZE_GLOBAL_FOR_FEATURE(harmony_trailing_commas)
+EMPTY_INITIALIZE_GLOBAL_FOR_FEATURE(harmony_function_tostring)
 EMPTY_INITIALIZE_GLOBAL_FOR_FEATURE(harmony_class_fields)
 EMPTY_INITIALIZE_GLOBAL_FOR_FEATURE(harmony_object_rest_spread)
 EMPTY_INITIALIZE_GLOBAL_FOR_FEATURE(harmony_dynamic_import)
@@ -4124,6 +4125,7 @@ bool Genesis::InstallExperimentalNatives() {
 #endif
   static const char* harmony_restrictive_generators_natives[] = {nullptr};
   static const char* harmony_trailing_commas_natives[] = {nullptr};
+  static const char* harmony_function_tostring_natives[] = {nullptr};
   static const char* harmony_class_fields_natives[] = {nullptr};
   static const char* harmony_object_rest_spread_natives[] = {nullptr};
   static const char* harmony_async_iteration_natives[] = {nullptr};

[Yang, 2017-02-16 18:01:50]

On 2017/02/16 15:07:25, commit-bot: I haz the power wrote:
> Failed to apply patch for src/bootstrapper.cc:
> While running git apply --index -p1;
>   error: patch failed: src/bootstrapper.cc:3532
>   error: src/bootstrapper.cc: patch does not apply
> 
> Patch:       src/bootstrapper.cc
> Index: src/bootstrapper.cc
> diff --git a/src/bootstrapper.cc b/src/bootstrapper.cc
> index
>
dbc7f928e8c6a0958b78f9f6bf3add886d6be14a..dda24fe9e37f1c5dfcace806b8a36abca1e47577
> 100644
> --- a/src/bootstrapper.cc
> +++ b/src/bootstrapper.cc
> @@ -3532,6 +3532,7 @@ EMPTY_INITIALIZE_GLOBAL_FOR_FEATURE(icu_case_mapping)
>  #endif
>  EMPTY_INITIALIZE_GLOBAL_FOR_FEATURE(harmony_restrictive_generators)
>  EMPTY_INITIALIZE_GLOBAL_FOR_FEATURE(harmony_trailing_commas)
> +EMPTY_INITIALIZE_GLOBAL_FOR_FEATURE(harmony_function_tostring)
>  EMPTY_INITIALIZE_GLOBAL_FOR_FEATURE(harmony_class_fields)
>  EMPTY_INITIALIZE_GLOBAL_FOR_FEATURE(harmony_object_rest_spread)
>  EMPTY_INITIALIZE_GLOBAL_FOR_FEATURE(harmony_dynamic_import)
> @@ -4124,6 +4125,7 @@ bool Genesis::InstallExperimentalNatives() {
>  #endif
>    static const char* harmony_restrictive_generators_natives[] = {nullptr};
>    static const char* harmony_trailing_commas_natives[] = {nullptr};
> +  static const char* harmony_function_tostring_natives[] = {nullptr};
>    static const char* harmony_class_fields_natives[] = {nullptr};
>    static const char* harmony_object_rest_spread_natives[] = {nullptr};
>    static const char* harmony_async_iteration_natives[] = {nullptr};

oh yeah. please rename the CL.

[jwolfe, 2017-02-16 19:51:31]

The CQ bit was checked by jwolfe@igalia.com

[jwolfe, 2017-02-16 19:51:32]

The patchset sent to the CQ was uploaded after l-g-t-m from
littledan@chromium.org, yangguo@chromium.org
Link to the patchset: https://codereview.chromium.org/2156303002/#ps320001
(title: "rebase")

[commit-bot: I haz the power, 2017-02-16 19:51:43]

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-02-16 20:19:25]

CQ is committing da patch.

Bot data: {"patchset_id": 320001, "attempt_start_ts": 1487274691979790,
"parent_rev": "0393b11deab1a77716770d67c7009f7fc9f57f44", "commit_rev":
"d1d4b9ce51d2ef3e9bd760e70642acddac30846f"}

[commit-bot: I haz the power, 2017-02-16 20:19:33]

Description was changed from

==========
Implement new Function.prototype.toString --harmony-function-tostring

For functions declared in source code, the .toString() representation
will be an excerpt of the source code.
* For functions declared with the "function" keyword, the excerpt
  starts at the "function" or "async" keyword and ends at the final "}".
  The previous behavior would start the excerpt at the "(" of the
  parameter list, and prepend a canonical `"function " + name` or
  similar, which would discard comments and formatting surrounding the
  function's name. Anonymous functions declared as function expressions
  no longer get the name "anonymous" in their toString representation.
* For methods, the excerpt starts at the "get", "set", "*" (for
  generator methods), or property name, whichever comes first.
  Previously, the toString representation for methods would use a
  canonical prefix before the "(" of the parameter list. Note that any
  "static" keyword is omitted.
* For arrow functions and class declarations, the excerpt is unchanged.

For functions created with the Function, GeneratorFunction, or
AsyncFunction constructors:
* The string separating the parameter text and body text is now
  "\n) {\n", where previously it was "\n/*``*/) {\n" or ") {\n".
* At one point, newline normalization was required by the spec here,
  but that was removed from the spec, and so this CL does not do it.

Included in this CL is a fix for CreateDynamicFunction parsing. ')'
and '`' characters in the parameter string are no longer disallowed,
and Function("a=function(", "}){") is no longer allowed.

BUG=v8:4958, v8:4230
==========

to

==========
Implement new Function.prototype.toString --harmony-function-tostring

For functions declared in source code, the .toString() representation
will be an excerpt of the source code.
* For functions declared with the "function" keyword, the excerpt
  starts at the "function" or "async" keyword and ends at the final "}".
  The previous behavior would start the excerpt at the "(" of the
  parameter list, and prepend a canonical `"function " + name` or
  similar, which would discard comments and formatting surrounding the
  function's name. Anonymous functions declared as function expressions
  no longer get the name "anonymous" in their toString representation.
* For methods, the excerpt starts at the "get", "set", "*" (for
  generator methods), or property name, whichever comes first.
  Previously, the toString representation for methods would use a
  canonical prefix before the "(" of the parameter list. Note that any
  "static" keyword is omitted.
* For arrow functions and class declarations, the excerpt is unchanged.

For functions created with the Function, GeneratorFunction, or
AsyncFunction constructors:
* The string separating the parameter text and body text is now
  "\n) {\n", where previously it was "\n/*``*/) {\n" or ") {\n".
* At one point, newline normalization was required by the spec here,
  but that was removed from the spec, and so this CL does not do it.

Included in this CL is a fix for CreateDynamicFunction parsing. ')'
and '`' characters in the parameter string are no longer disallowed,
and Function("a=function(", "}){") is no longer allowed.

BUG=v8:4958, v8:4230

Review-Url: https://codereview.chromium.org/2156303002
Cr-Commit-Position: refs/heads/master@{#43262}
Committed:
https://chromium.googlesource.com/v8/v8/+/d1d4b9ce51d2ef3e9bd760e70642acddac3...
==========

[commit-bot: I haz the power, 2017-02-16 20:19:34]

Committed patchset #17 (id:320001) as
https://chromium.googlesource.com/v8/v8/+/d1d4b9ce51d2ef3e9bd760e70642acddac3...