Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(3187)

Issue 2156303002: Implement new Function.prototype.toString and fix CreateDynamicFunction parsing (Closed)

Created:
3 years, 6 months ago by jwolfe
Modified:
2 years, 11 months ago
Reviewers:
Dan Ehrenberg, Yang
CC:
Michael Hablich, v8-reviews_googlegroups.com
Base URL:
https://chromium.googlesource.com/v8/v8.git@master
Target Ref:
refs/pending/heads/master
Project:
v8
Visibility:
Public.

Description

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

Patch Set 1 #

Total comments: 2

Patch Set 2 : add test. fix unicode bug #

Patch Set 3 : more tests and bug fixes #

Patch Set 4 : rebase (still WIP) #

Patch Set 5 : add fixes for v8:4230. still needs work. #

Patch Set 6 : fix async function constructor #

Total comments: 1

Patch Set 7 : remove newline normalization #

Patch Set 8 : rebase #

Total comments: 12

Patch Set 9 : address review comments #

Patch Set 10 : rebase #

Patch Set 11 : rename scope_position to position #

Total comments: 1

Patch Set 12 : rebase #

Patch Set 13 : update test262.status and implement new CreateDynamicFunction() behavior #

Patch Set 14 : rebase #

Total comments: 4

Patch Set 15 : add DCHECK for the name anonymous #

Total comments: 1

Patch Set 16 : update comment for removing anonymous identifier #

Patch Set 17 : rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+389 lines, -120 lines) Patch
M src/api.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 3 chunks +23 lines, -6 lines 0 comments Download
M src/bootstrapper.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 2 chunks +2 lines, -0 lines 0 comments Download
M src/builtins/builtins-function.cc View 1 2 3 4 5 6 7 8 9 10 11 12 3 chunks +33 lines, -19 lines 0 comments Download
M src/builtins/builtins-global.cc View 1 2 3 4 5 6 7 8 9 1 chunk +4 lines, -3 lines 0 comments Download
M src/compilation-cache.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 3 chunks +4 lines, -5 lines 0 comments Download
M src/compilation-cache.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 5 chunks +12 lines, -13 lines 0 comments Download
M src/compiler.h View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +4 lines, -4 lines 0 comments Download
M src/compiler.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 4 chunks +25 lines, -6 lines 0 comments Download
M src/debug/debug-evaluate.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +1 line, -1 line 0 comments Download
M src/flag-definitions.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +1 line, -0 lines 0 comments Download
M src/messages.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +3 lines, -0 lines 0 comments Download
M src/objects.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 3 chunks +3 lines, -3 lines 0 comments Download
M src/objects.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 11 chunks +38 lines, -19 lines 0 comments Download
M src/parsing/parse-info.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 2 chunks +6 lines, -0 lines 0 comments Download
M src/parsing/parse-info.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +1 line, -0 lines 0 comments Download
M src/parsing/parser.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 2 chunks +12 lines, -0 lines 0 comments Download
M src/parsing/parser.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 3 chunks +29 lines, -1 line 0 comments Download
M src/parsing/parser-base.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 7 chunks +28 lines, -10 lines 0 comments Download
M src/parsing/preparser.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +2 lines, -0 lines 0 comments Download
M src/runtime/runtime-compiler.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +4 lines, -3 lines 0 comments Download
M src/string-builder.h View 1 2 3 4 1 chunk +2 lines, -0 lines 0 comments Download
M test/cctest/wasm/test-wasm-breakpoints.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +2 lines, -1 line 0 comments Download
A test/mjsunit/harmony/function-tostring.js View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +124 lines, -0 lines 0 comments Download
M test/test262/test262.status View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +26 lines, -26 lines 0 comments Download

Messages

Total messages: 67 (34 generated)
jwolfe
Would it be a good idea or a waste of time to add tests to ...
3 years, 6 months ago (2016-07-18 18:33:10 UTC) #3
Dan Ehrenberg
I don't think we need to include tests which are redundant with test262 tests beyond ...
3 years, 6 months ago (2016-07-18 18:52:32 UTC) #4
jwolfe
Sure enough, mjsunit/regress/regress-2470 fails with this flag enabled. I think the only real solution to ...
3 years, 6 months ago (2016-07-18 22:56:41 UTC) #5
Dan Ehrenberg
On 2016/07/18 at 22:56:41, jwolfe wrote: > Sure enough, mjsunit/regress/regress-2470 fails with this flag enabled. ...
3 years, 6 months ago (2016-07-19 22:42:48 UTC) #6
jwolfe
I added a test for both --harmony-async-await and --harmony-function-tostring working together. Is that the right ...
3 years, 6 months ago (2016-07-20 04:14:09 UTC) #8
jwolfe
Results of running all the tests with this flag on: 11 of the tests were ...
3 years, 6 months ago (2016-07-20 05:44:02 UTC) #10
jwolfe
> I'd like to submit a CL where I make whitespace changes to those 11 ...
3 years, 6 months ago (2016-07-20 06:17:27 UTC) #11
Dan Ehrenberg
How's this patch going? Is it ready for a full review, or is it still ...
3 years, 3 months ago (2016-10-26 10:27:25 UTC) #14
jwolfe
It looks like there's a viable, straight-forward approach to solving v8:4230 in this CL. This ...
3 years, 3 months ago (2016-10-28 20:13:55 UTC) #18
jwolfe
PTAL. I've still got some test work to do, but I believe the code here ...
3 years, 2 months ago (2016-11-01 21:34:19 UTC) #20
caitp
On 2016/07/18 18:52:32, Dan Ehrenberg wrote: > I don't think we need to include tests ...
3 years, 2 months ago (2016-11-02 02:41:32 UTC) #21
jwolfe
The following test fails with --harmony-funtion-tostring: mjsunit/regress/regress-2470 This is due to this code right here: ...
3 years, 1 month ago (2016-12-01 18:52:54 UTC) #22
Dan Ehrenberg
On 2016/12/01 18:52:54, jwolfe wrote: > The following test fails with --harmony-funtion-tostring: > mjsunit/regress/regress-2470 > ...
3 years, 1 month ago (2016-12-01 18:54:32 UTC) #23
jwolfe
PTAL. The following microbenchmark demonstrates no performance regression in Function(), and a significant performance improvement ...
3 years, 1 month ago (2016-12-02 00:51:25 UTC) #25
Dan Ehrenberg
I really like this implementation strategy, and it's great news that implementing the new semantics ...
3 years, 1 month ago (2016-12-06 00:32:13 UTC) #26
jwolfe
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 ...
3 years ago (2017-01-13 00:28:48 UTC) #27
jwolfe
PTAL. I used the name "position" instead of "scope_position" per your suggestion.
3 years ago (2017-01-13 22:53:42 UTC) #28
Dan Ehrenberg
Could you enable test262 tests that this fixes? See https://docs.google.com/document/d/16bj7AIDgZLv4WOsUEzQ5NzcEN9_xo095e88Pz8FC5rA/edit?pli=1 for context. (Sorry, this was ...
3 years ago (2017-01-14 00:04:44 UTC) #29
Dan Ehrenberg
Could you enable test262 tests that this fixes? See https://docs.google.com/document/d/16bj7AIDgZLv4WOsUEzQ5NzcEN9_xo095e88Pz8FC5rA/edit?pli=1 for context. (Sorry, this was ...
3 years ago (2017-01-14 00:04:47 UTC) #30
jwolfe
PTAL. On 2017/01/14 00:04:47, Dan Ehrenberg wrote: > Could you enable test262 tests that this ...
2 years, 11 months ago (2017-02-04 00:48:11 UTC) #31
Dan Ehrenberg
The CQ seems to show a crash; do you know what this is? Thanks for ...
2 years, 11 months ago (2017-02-05 10:19:38 UTC) #36
Dan Ehrenberg
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-base.h#newcode3431 src/parsing/parser-base.h:3431: Consume(Token::IDENTIFIER); Optional: You could put a DCHECK here that ...
2 years, 11 months ago (2017-02-13 17:32:32 UTC) #45
jwolfe
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-base.h#newcode3431 src/parsing/parser-base.h:3431: Consume(Token::IDENTIFIER); On 2017/02/13 17:32:32, Dan Ehrenberg wrote: > Optional: ...
2 years, 11 months ago (2017-02-13 21:31:05 UTC) #46
Dan Ehrenberg
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-base.h#newcode3430 ...
2 years, 11 months ago (2017-02-14 12:06:00 UTC) #47
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2156303002/300001
2 years, 11 months ago (2017-02-14 20:16:49 UTC) #50
commit-bot: I haz the power
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)
2 years, 11 months ago (2017-02-14 20:20:00 UTC) #52
Dan Ehrenberg
Adding Yang as a debug owner
2 years, 11 months ago (2017-02-14 20:44:43 UTC) #55
Yang
On 2017/02/14 20:44:43, Dan Ehrenberg wrote: > Adding Yang as a debug owner src/debug lgtm.
2 years, 11 months ago (2017-02-16 14:14:38 UTC) #56
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2156303002/300001
2 years, 11 months ago (2017-02-16 14:36:56 UTC) #58
commit-bot: I haz the power
Failed to apply patch for src/bootstrapper.cc: While running git apply --index -p1; error: patch failed: ...
2 years, 11 months ago (2017-02-16 15:07:25 UTC) #60
Yang
On 2017/02/16 15:07:25, commit-bot: I haz the power wrote: > Failed to apply patch for ...
2 years, 11 months ago (2017-02-16 18:01:50 UTC) #61
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2156303002/320001
2 years, 11 months ago (2017-02-16 19:51:43 UTC) #64
commit-bot: I haz the power
2 years, 11 months ago (2017-02-16 20:19:34 UTC) #67
Message was sent while issue was closed.
Committed patchset #17 (id:320001) as
https://chromium.googlesource.com/v8/v8/+/d1d4b9ce51d2ef3e9bd760e70642acddac3...

Powered by Google App Engine
This is Rietveld 408576698