Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(737)

Issue 2235423003: [parser] improve inferred function names for async arrow functions (Closed)

Created:
4 years, 4 months ago by caitp
Modified:
4 years, 4 months ago
Reviewers:
Dan Ehrenberg, adamk, marja
CC:
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

[parser] improve inferred function names for async arrow functions No longer include the "async" keyword, or an async arrow function's single identifier parameter as part of its inferred name. BUG=v8:5281, v8:4483 R=adamk@chromium.org, littledan@chromium.org, marja@chromium.org Committed: https://crrev.com/a9e470797be50ea39ec0caeedfa609564330141b Cr-Commit-Position: refs/heads/master@{#38627}

Patch Set 1 #

Total comments: 3

Patch Set 2 : Add a bunch more tests #

Patch Set 3 : Try to make it a bit nicer + add some dchecks #

Total comments: 1

Patch Set 4 : FuncNameInferrer::RemoveAsyncKeywordAtIndex() #

Patch Set 5 : a better/cheaper mechanic #

Total comments: 4

Patch Set 6 : last of it #

Patch Set 7 : really the last of it #

Total comments: 2

Patch Set 8 : last nits #

Patch Set 9 : add braces around single-line if stmt #

Unified diffs Side-by-side diffs Delta from patch set Stats (+124 lines, -40 lines) Patch
M src/parsing/func-name-inferrer.h View 1 2 3 4 5 2 chunks +4 lines, -0 lines 0 comments Download
M src/parsing/func-name-inferrer.cc View 1 2 3 4 5 6 1 chunk +5 lines, -0 lines 0 comments Download
M src/parsing/parser.h View 1 2 1 chunk +2 lines, -1 line 0 comments Download
M src/parsing/parser.cc View 1 2 3 4 5 6 7 8 1 chunk +5 lines, -2 lines 0 comments Download
M src/parsing/parser-base.h View 1 2 3 4 5 6 7 2 chunks +12 lines, -4 lines 0 comments Download
M src/parsing/preparser.h View 1 2 1 chunk +3 lines, -3 lines 0 comments Download
M test/cctest/test-parsing.cc View 1 2 3 4 1 chunk +23 lines, -23 lines 0 comments Download
M test/mjsunit/harmony/async-function-stacktrace.js View 1 3 chunks +70 lines, -7 lines 0 comments Download

Messages

Total messages: 42 (25 generated)
caitp
Hi, PTAL. I believe there's probably a slightly cleaner way to do this, as ordinary ...
4 years, 4 months ago (2016-08-11 23:09:21 UTC) #3
caitp
On 2016/08/11 23:09:21, caitp wrote: > Hi, PTAL. > > I believe there's probably a ...
4 years, 4 months ago (2016-08-11 23:18:15 UTC) #4
Dan Ehrenberg
https://codereview.chromium.org/2235423003/diff/1/src/parsing/parser-base.h File src/parsing/parser-base.h (right): https://codereview.chromium.org/2235423003/diff/1/src/parsing/parser-base.h#newcode2333 src/parsing/parser-base.h:2333: fni_->RemoveLastName(); I'm surprised that you have to remove two ...
4 years, 4 months ago (2016-08-12 00:39:21 UTC) #7
caitp
https://codereview.chromium.org/2235423003/diff/1/src/parsing/parser-base.h File src/parsing/parser-base.h (right): https://codereview.chromium.org/2235423003/diff/1/src/parsing/parser-base.h#newcode2333 src/parsing/parser-base.h:2333: fni_->RemoveLastName(); On 2016/08/12 00:39:21, Dan Ehrenberg wrote: > I'm ...
4 years, 4 months ago (2016-08-12 01:12:27 UTC) #8
caitp
added more tests. I'll try making it more elegant tomorrow, but as mentioned inline, it ...
4 years, 4 months ago (2016-08-12 01:25:00 UTC) #9
marja
Can you post a link to the relevant parts of the spec (if any) where ...
4 years, 4 months ago (2016-08-12 09:20:56 UTC) #14
caitp
On 2016/08/12 09:20:56, marja wrote: > Can you post a link to the relevant parts ...
4 years, 4 months ago (2016-08-12 13:33:00 UTC) #15
caitp
There are a lot of tests which depend on the current behaviour of the FuncNameInferrer, ...
4 years, 4 months ago (2016-08-12 16:57:13 UTC) #16
Dan Ehrenberg
Seems like it's genuinely hard to do this without going back and removing it, as ...
4 years, 4 months ago (2016-08-12 18:16:41 UTC) #21
Dan Ehrenberg
I'm confortable with the logic now (seems unavoidable, unfortunately, to back up and remove the ...
4 years, 4 months ago (2016-08-12 21:29:58 UTC) #28
caitp
https://codereview.chromium.org/2235423003/diff/80001/src/parsing/func-name-inferrer.h File src/parsing/func-name-inferrer.h (right): https://codereview.chromium.org/2235423003/diff/80001/src/parsing/func-name-inferrer.h#newcode78 src/parsing/func-name-inferrer.h:78: void RemoveAsyncKeywordAtIndex(int index); On 2016/08/12 21:29:58, Dan Ehrenberg wrote: ...
4 years, 4 months ago (2016-08-12 21:32:45 UTC) #29
Dan Ehrenberg
https://codereview.chromium.org/2235423003/diff/80001/src/parsing/func-name-inferrer.h File src/parsing/func-name-inferrer.h (right): https://codereview.chromium.org/2235423003/diff/80001/src/parsing/func-name-inferrer.h#newcode78 src/parsing/func-name-inferrer.h:78: void RemoveAsyncKeywordAtIndex(int index); On 2016/08/12 at 21:32:45, caitp wrote: ...
4 years, 4 months ago (2016-08-12 21:35:28 UTC) #30
Dan Ehrenberg
lgtm https://codereview.chromium.org/2235423003/diff/120001/src/parsing/parser.cc File src/parsing/parser.cc (left): https://codereview.chromium.org/2235423003/diff/120001/src/parsing/parser.cc#oldcode774 src/parsing/parser.cc:774: Nit: revert whitespace change
4 years, 4 months ago (2016-08-12 22:07:18 UTC) #35
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/2235423003/160001
4 years, 4 months ago (2016-08-12 22:18:48 UTC) #38
caitp
https://codereview.chromium.org/2235423003/diff/120001/src/parsing/parser.cc File src/parsing/parser.cc (left): https://codereview.chromium.org/2235423003/diff/120001/src/parsing/parser.cc#oldcode774 src/parsing/parser.cc:774: On 2016/08/12 22:07:18, Dan Ehrenberg wrote: > Nit: revert ...
4 years, 4 months ago (2016-08-12 22:19:09 UTC) #39
commit-bot: I haz the power
Committed patchset #9 (id:160001)
4 years, 4 months ago (2016-08-12 22:47:02 UTC) #40
commit-bot: I haz the power
4 years, 4 months ago (2016-08-12 22:47:23 UTC) #42
Message was sent while issue was closed.
Patchset 9 (id:??) landed as
https://crrev.com/a9e470797be50ea39ec0caeedfa609564330141b
Cr-Commit-Position: refs/heads/master@{#38627}

Powered by Google App Engine
This is Rietveld 408576698