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

Issue 1104223002: [es6] implement optional parameters via desugaring (with scoping) (Closed)

Created:
4 years, 4 months ago by caitp (gmail)
Modified:
4 years ago
CC:
v8-dev
Base URL:
https://chromium.googlesource.com/v8/v8.git@master
Target Ref:
refs/pending/heads/master
Project:
v8
Visibility:
Public.

Description

[es6] implement optional parameters via desugaring TODO: - make evaluation work for arrow fns - left-to-right parameter TDZ BUG=v8:2160 R= LOG=N

Patch Set 1 #

Patch Set 2 : Make scoping sort-of work-ish, still no hole-checking #

Total comments: 1

Patch Set 3 : Wrap parameter scope around inner function body scope #

Patch Set 4 : Remove debug helpers :d #

Patch Set 5 : Finalize function body scope if it's not needed #

Total comments: 8

Patch Set 6 : Maybe fixed #

Patch Set 7 : Slight fixup + initial test coverage #

Patch Set 8 : Additional coverage + TODOs #

Patch Set 9 : git cl format #

Patch Set 10 : Do hole-checking in parameters (ugly) #

Patch Set 11 : Add a debugger test #

Unified diffs Side-by-side diffs Delta from patch set Stats (+488 lines, -75 lines) Patch
M src/globals.h View 1 2 1 chunk +9 lines, -8 lines 0 comments Download
M src/parser.h View 1 2 3 4 5 6 7 8 9 3 chunks +17 lines, -4 lines 0 comments Download
M src/parser.cc View 1 2 3 4 5 6 7 8 9 10 13 chunks +172 lines, -29 lines 0 comments Download
M src/preparser.h View 1 2 3 4 5 6 7 8 9 10 9 chunks +40 lines, -12 lines 0 comments Download
M src/preparser.cc View 1 2 3 4 5 6 7 8 9 4 chunks +31 lines, -7 lines 0 comments Download
M src/runtime/runtime-debug.cc View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M src/scopes.h View 1 2 3 4 5 6 7 8 9 10 chunks +24 lines, -7 lines 0 comments Download
M src/scopes.cc View 1 2 3 4 5 6 7 8 9 19 chunks +54 lines, -8 lines 0 comments Download
M src/variables.cc View 1 2 3 4 5 6 7 8 1 chunk +9 lines, -0 lines 0 comments Download
A test/mjsunit/harmony/optional-arguments.js View 1 2 3 4 5 6 7 8 9 1 chunk +67 lines, -0 lines 0 comments Download
A test/mjsunit/harmony/optional-arguments-debug.js View 1 2 3 4 5 6 7 8 9 10 1 chunk +64 lines, -0 lines 0 comments Download

Messages

Total messages: 24 (4 generated)
caitp (gmail)
PTAL --- need ideas on doing the right thing for TDZ, and scoping this properly. ...
4 years, 4 months ago (2015-04-27 15:43:37 UTC) #1
caitp (gmail)
dslomov@ it might be good to coordinate on the desugaring given crrev.com/1107053002
4 years, 4 months ago (2015-04-27 17:19:00 UTC) #3
Dmitry Lomov (no reviews)
High-level bit: can I haz a dezign docz? :) I am but a simple engineer ...
4 years, 4 months ago (2015-04-28 08:57:09 UTC) #6
caitp (gmail)
On 2015/04/28 08:57:09, Dmitry Lomov (chromium) wrote: > High-level bit: can I haz a dezign ...
4 years, 4 months ago (2015-04-28 13:08:01 UTC) #7
caitp (gmail)
On 2015/04/28 13:08:01, caitp wrote: > On 2015/04/28 08:57:09, Dmitry Lomov (chromium) wrote: > > ...
4 years, 4 months ago (2015-04-28 14:19:19 UTC) #8
Dmitry Lomov (no reviews)
> > I put together a "design" doc at > https://docs.google.com/document/d/1FLzjjRDKKLOjOiP2vLofmBHmYoP2RUDeNJorH5MfkCk/edit?usp=sharing > > I'm really ...
4 years, 4 months ago (2015-04-29 09:31:24 UTC) #9
caitp (gmail)
Okay, so: In this patch, a new declaration scope is used for the inner function ...
4 years, 4 months ago (2015-05-01 14:59:20 UTC) #10
arv (Not doing code reviews)
It is not yet clear how to best structure these scopes. https://codereview.chromium.org/1104223002/diff/80001/src/parser.cc File src/parser.cc (right): ...
4 years, 4 months ago (2015-05-01 18:31:33 UTC) #11
caitp (gmail)
https://codereview.chromium.org/1104223002/diff/80001/src/parser.cc File src/parser.cc (right): https://codereview.chromium.org/1104223002/diff/80001/src/parser.cc#newcode4208 src/parser.cc:4208: if (func_scope != function_body) { On 2015/05/01 18:31:32, arv ...
4 years, 4 months ago (2015-05-01 19:03:35 UTC) #12
arv (Not doing code reviews)
On 2015/05/01 19:03:35, caitp wrote: > Is what you're saying "make the parameter scope a ...
4 years, 4 months ago (2015-05-01 19:06:20 UTC) #13
caitp (gmail)
On 2015/05/01 19:06:20, arv wrote: > On 2015/05/01 19:03:35, caitp wrote: > > > Is ...
4 years, 4 months ago (2015-05-01 19:10:06 UTC) #14
caitp (gmail)
I think I've got the variable allocation issue resolved, just needs more test coverage. There ...
4 years, 4 months ago (2015-05-04 15:40:58 UTC) #15
caitp (gmail)
First stab at TDZ behaviour for parameters is done There's an issue with lazy parsing, ...
4 years, 4 months ago (2015-05-04 21:36:09 UTC) #16
rossberg
I'm still concerned about the whole scoping business. Scoping in V8 is a mess and ...
4 years, 4 months ago (2015-05-05 13:55:20 UTC) #17
caitp (gmail)
On 2015/05/05 13:55:20, rossberg wrote: > I'm still concerned about the whole scoping business. Scoping ...
4 years, 4 months ago (2015-05-05 14:21:38 UTC) #18
arv (Not doing code reviews)
On 2015/05/05 14:21:38, caitp wrote: > On 2015/05/05 13:55:20, rossberg wrote: > > I'm still ...
4 years, 4 months ago (2015-05-05 14:38:45 UTC) #19
caitp (gmail)
What if the debugger behaviour just gets some test coverage so that you're more confident ...
4 years, 4 months ago (2015-05-05 14:48:22 UTC) #20
rossberg
On 2015/05/05 14:38:45, arv wrote: > On 2015/05/05 14:21:38, caitp wrote: > > On 2015/05/05 ...
4 years, 4 months ago (2015-05-05 15:22:23 UTC) #21
rossberg
On 2015/05/05 14:48:22, caitp wrote: > What if the debugger behaviour just gets some test ...
4 years, 4 months ago (2015-05-05 15:30:08 UTC) #22
caitp (gmail)
4 years, 4 months ago (2015-05-05 18:31:44 UTC) #24
On 2015/05/05 15:30:08, rossberg wrote:
> On 2015/05/05 14:48:22, caitp wrote:
> > What if the debugger behaviour just gets some test coverage so that you're
> more
> > confident with the scoped approach? I can try to do that
> 
> I really want to separate concerns. As I said, scoping is awfully tricky in V8
> (just ask Dmitry or Andy, who both had many weeks of fun with it recently). We
> should keep changes in that area minimal and incremental. When touching it is
> unavoidable, we should do it separately from as much else as possible.

Scopes are lots of fun, particularly whatever the issue that's breaking lazy
parsing is. But honestly the changes here to scopes are not all that
substantial. Adding arrow functions will break the rules for shadowing, and will
probably break default params in class constructors too. I'm just not sure it's
worth dealing with those extra bugs.

I'm working on a version like that, but I foresee there are going to be
problems, this one is closer to being correct and workable

Powered by Google App Engine
This is Rietveld 408576698