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

Issue 2077283004: Rewrite scopes of non-simple default arguments (Closed)

Created:
3 years ago by Dan Ehrenberg
Modified:
3 years ago
Reviewers:
adamk
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

Rewrite scopes of non-simple default arguments Default parameters have additional declaration block scopes inserted around them when something in the function scope calls eval. This patch sets the parent scope of the expressions introduced due to those defaults to the new block scope. R=adamk BUG=chromium:616386 Committed: https://crrev.com/0e14baf712955a1993f742647bb2adc293702b80 Cr-Commit-Position: refs/heads/master@{#37198}

Patch Set 1 #

Patch Set 2 : Code cleanup and more tests #

Patch Set 3 : Fix remaining edge case #

Patch Set 4 : revert irrelevant whitespace change #

Patch Set 5 : Fix whitespace #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+41 lines, -1 line) Patch
M src/parsing/parameter-initializer-rewriter.cc View 1 2 1 chunk +4 lines, -0 lines 0 comments Download
M src/parsing/parser.cc View 1 2 3 1 chunk +6 lines, -0 lines 0 comments Download
M src/parsing/pattern-rewriter.cc View 1 2 1 chunk +21 lines, -1 line 1 comment Download
A test/mjsunit/regress/regress-616386.js View 1 2 3 4 1 chunk +10 lines, -0 lines 0 comments Download

Messages

Total messages: 19 (7 generated)
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2077283004/40001
3 years ago (2016-06-21 21:27:43 UTC) #2
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2077283004/60001
3 years ago (2016-06-21 21:29:04 UTC) #4
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: v8_presubmit on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/17704)
3 years ago (2016-06-21 21:32:24 UTC) #6
Dan Ehrenberg
3 years ago (2016-06-21 22:06:04 UTC) #7
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2077283004/80001
3 years ago (2016-06-21 23:12:59 UTC) #9
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
3 years ago (2016-06-21 23:40:04 UTC) #11
adamk
I'm happy to see the bug fixed, but wonder what the next step for making ...
3 years ago (2016-06-22 17:55:39 UTC) #12
adamk
lgtm to get this fixed expediently, and minimally (for back-porting purposes).
3 years ago (2016-06-22 18:16:43 UTC) #13
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2077283004/80001
3 years ago (2016-06-22 18:18:25 UTC) #15
commit-bot: I haz the power
Committed patchset #5 (id:80001)
3 years ago (2016-06-22 18:20:13 UTC) #16
commit-bot: I haz the power
Patchset 5 (id:??) landed as https://crrev.com/0e14baf712955a1993f742647bb2adc293702b80 Cr-Commit-Position: refs/heads/master@{#37198}
3 years ago (2016-06-22 18:22:29 UTC) #18
Dan Ehrenberg
3 years ago (2016-06-22 19:57:18 UTC) #19
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/2081323006/ by littledan@chromium.org.

The reason for reverting is: Seems to close tree (but it could be an infra
issue).

Powered by Google App Engine
This is Rietveld 408576698