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

Issue 2176653003: Wrap ClassLiterals in DoExpressions instead of giving them BlockScopes. (Closed)

Created:
4 years, 5 months ago by bakkot
Modified:
4 years, 5 months ago
CC:
oth, rmcilroy, 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

Wrap ClassLiterals in DoExpressions instead of giving them BlockScopes. This slightly simplifies scope handling. It also makes it possible to implement some potential future changes to classes purely in the parser by adding additional code to the DoExpression. This is a portion of https://codereview.chromium.org/2142333002/, which probably isn't going through in full. Committed: https://crrev.com/c2bcfc31454bfa95e146cb6002fb6a9a142181f7 Cr-Commit-Position: refs/heads/master@{#38035}

Patch Set 1 #

Patch Set 2 : cleanup source positions #

Patch Set 3 : update bytecode expecatations #

Patch Set 4 : rebase #

Total comments: 4

Patch Set 5 : address comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+167 lines, -200 lines) Patch
M src/ast/ast.h View 1 2 3 4 7 chunks +18 lines, -19 lines 0 comments Download
M src/ast/ast.cc View 1 2 3 4 2 chunks +8 lines, -2 lines 0 comments Download
M src/compiler/ast-graph-builder.h View 1 2 3 1 chunk +0 lines, -3 lines 0 comments Download
M src/compiler/ast-graph-builder.cc View 1 2 3 1 chunk +0 lines, -14 lines 0 comments Download
M src/full-codegen/full-codegen.h View 1 2 3 1 chunk +0 lines, -17 lines 0 comments Download
M src/full-codegen/full-codegen.cc View 1 2 3 4 1 chunk +25 lines, -32 lines 0 comments Download
M src/interpreter/bytecode-generator.h View 1 2 3 4 1 chunk +0 lines, -1 line 0 comments Download
M src/interpreter/bytecode-generator.cc View 1 2 3 4 1 chunk +0 lines, -12 lines 0 comments Download
M src/parsing/parameter-initializer-rewriter.cc View 1 2 3 4 1 chunk +0 lines, -1 line 0 comments Download
M src/parsing/parser.h View 1 2 3 4 2 chunks +10 lines, -10 lines 0 comments Download
M src/parsing/parser.cc View 1 2 3 4 5 chunks +25 lines, -12 lines 0 comments Download
M test/cctest/interpreter/bytecode_expectations/ClassDeclarations.golden View 1 2 3 4 5 chunks +81 lines, -77 lines 0 comments Download

Messages

Total messages: 35 (20 generated)
bakkot
4 years, 5 months ago (2016-07-22 20:02:01 UTC) #11
adamk
+mstarzinger for compile and interpreter
4 years, 5 months ago (2016-07-22 23:35:47 UTC) #15
adamk
This seems like a good idea to me; note that I just landed a change ...
4 years, 5 months ago (2016-07-22 23:37:53 UTC) #16
bakkot
On 2016/07/22 at 23:37:53, adamk wrote: > This seems like a good idea to me; ...
4 years, 5 months ago (2016-07-23 01:14:16 UTC) #17
marja
src/parsing/parser* LGTM, as the changes there are straightforward. adamk@ can review parsing/pattern-rewriter.cc as I'm not ...
4 years, 5 months ago (2016-07-25 08:02:41 UTC) #18
Michael Starzinger
LGTM on everything aside from the parser. Just one nit left. Thanks! https://codereview.chromium.org/2176653003/diff/60001/src/full-codegen/full-codegen.cc File src/full-codegen/full-codegen.cc ...
4 years, 5 months ago (2016-07-25 14:49:54 UTC) #19
Michael Starzinger
One more comment. https://codereview.chromium.org/2176653003/diff/60001/src/ast/ast.h File src/ast/ast.h (right): https://codereview.chromium.org/2176653003/diff/60001/src/ast/ast.h#newcode2739 src/ast/ast.h:2739: BailoutId ExitId() { return BailoutId(local_id(2)); } ...
4 years, 5 months ago (2016-07-25 14:56:47 UTC) #20
adamk
On 2016/07/23 01:14:16, bakkot wrote: > On 2016/07/22 at 23:37:53, adamk wrote: > > This ...
4 years, 5 months ago (2016-07-25 17:19:28 UTC) #21
bakkot
On 2016/07/25 at 17:19:28, adamk wrote: > On 2016/07/23 01:14:16, bakkot wrote: > > On ...
4 years, 5 months ago (2016-07-25 18:51:48 UTC) #24
bakkot
https://codereview.chromium.org/2176653003/diff/60001/src/ast/ast.h File src/ast/ast.h (right): https://codereview.chromium.org/2176653003/diff/60001/src/ast/ast.h#newcode2739 src/ast/ast.h:2739: BailoutId ExitId() { return BailoutId(local_id(2)); } On 2016/07/25 at ...
4 years, 5 months ago (2016-07-25 18:52:04 UTC) #25
Dan Ehrenberg
lgtm
4 years, 5 months ago (2016-07-25 19:14:07 UTC) #28
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/2176653003/80001
4 years, 5 months ago (2016-07-25 19:14:46 UTC) #31
commit-bot: I haz the power
Committed patchset #5 (id:80001)
4 years, 5 months ago (2016-07-25 19:16:28 UTC) #32
commit-bot: I haz the power
Patchset 5 (id:??) landed as https://crrev.com/c2bcfc31454bfa95e146cb6002fb6a9a142181f7 Cr-Commit-Position: refs/heads/master@{#38035}
4 years, 5 months ago (2016-07-25 19:21:57 UTC) #34
Dan Ehrenberg
3 years, 7 months ago (2017-04-28 08:33:22 UTC) #35
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/2849773002/ by littledan@chromium.org.

The reason for reverting is: Blamed for
https://bugs.chromium.org/p/chromium/issues/detail?id=716265.

Powered by Google App Engine
This is Rietveld 408576698