[es6] Implement default parameters

[es6] Implement default parameters

Based on caitp's https://codereview.chromium.org/1127063003/

R=adamk@chromium.org, littledan@chromium.org
BUG=v8:2160
LOG=N

Committed: https://crrev.com/6dda11ff81a08c2790f963f100d94bba4a6f28b8
Cr-Commit-Position: refs/heads/master@{#30192}

[rossberg, 2015-08-12 15:20:28]

https://codereview.chromium.org/1287063004/diff/1/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/1287063004/diff/1/src/parser.cc#newcode3908
src/parser.cc:3908: // TODO(rossberg): allow initializers
Need to come up with a test that actually triggers this.

[adamk, 2015-08-12 18:22:59]

https://codereview.chromium.org/1287063004/diff/1/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/1287063004/diff/1/src/parser.cc#newcode4328
src/parser.cc:4328: Expression* param =
I think this should be called "initializer"

https://codereview.chromium.org/1287063004/diff/1/src/parser.cc#newcode4342
src/parser.cc:4342: parameter.pattern, parameter.pattern->position(), param);
The above-suggested renaming would make the callsite clearer.

https://codereview.chromium.org/1287063004/diff/1/src/preparser.h
File src/preparser.h (left):

https://codereview.chromium.org/1287063004/diff/1/src/preparser.h#oldcode3625
src/preparser.h:3625: bool is_rest, FormalParametersT* parameters,
This rest refactoring seems separate, could it be broken into another patch if
it's a prerequisite or dropped if it's a separate concern?

https://codereview.chromium.org/1287063004/diff/1/test/mjsunit/harmony/defaul...
File test/mjsunit/harmony/default-parameters.js (right):

https://codereview.chromium.org/1287063004/diff/1/test/mjsunit/harmony/defaul...
test/mjsunit/harmony/default-parameters.js:84: (function TestParameterScoping()
{
This test looks like it could use some cases with more complex evals, e.g.,
those that declare variables with "var" and "let" (in both strict and sloppy
modes).

[rossberg, 2015-08-13 11:06:37]

https://codereview.chromium.org/1287063004/diff/1/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/1287063004/diff/1/src/parser.cc#newcode4328
src/parser.cc:4328: Expression* param =
On 2015/08/12 18:22:59, adamk wrote:
> I think this should be called "initializer"

That would confuse it with the actual initializer expression. I renamed it to
initial_value instead, is that okay?

https://codereview.chromium.org/1287063004/diff/1/src/parser.cc#newcode4342
src/parser.cc:4342: parameter.pattern, parameter.pattern->position(), param);
On 2015/08/12 18:22:59, adamk wrote:
> The above-suggested renaming would make the callsite clearer.

Done.

https://codereview.chromium.org/1287063004/diff/1/src/preparser.h
File src/preparser.h (left):

https://codereview.chromium.org/1287063004/diff/1/src/preparser.h#oldcode3625
src/preparser.h:3625: bool is_rest, FormalParametersT* parameters,
On 2015/08/12 18:22:59, adamk wrote:
> This rest refactoring seems separate, could it be broken into another patch if
> it's a prerequisite or dropped if it's a separate concern?

Split into separate CL: https://codereview.chromium.org/1286133003

https://codereview.chromium.org/1287063004/diff/1/test/mjsunit/harmony/defaul...
File test/mjsunit/harmony/default-parameters.js (right):

https://codereview.chromium.org/1287063004/diff/1/test/mjsunit/harmony/defaul...
test/mjsunit/harmony/default-parameters.js:84: (function TestParameterScoping()
{
On 2015/08/12 18:22:59, adamk wrote:
> This test looks like it could use some cases with more complex evals, e.g.,
> those that declare variables with "var" and "let" (in both strict and sloppy
> modes).

Since these don't actually work correctly yet I just added a TODO.

[adamk, 2015-08-13 17:15:12]

lgtm (presuming you'll land the pulled-out patch first).

[rossberg, 2015-08-13 18:10:19]

The CQ bit was checked by rossberg@chromium.org

[commit-bot: I haz the power, 2015-08-13 18:10:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1287063004/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1287063004/20001

[commit-bot: I haz the power, 2015-08-13 18:11:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-13 18:11:19]

Try jobs failed on following builders:
  v8_linux_dbg on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg/builds/7599)

[rossberg, 2015-08-13 18:39:30]

The CQ bit was checked by rossberg@chromium.org

[commit-bot: I haz the power, 2015-08-13 18:39:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1287063004/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1287063004/20001

[commit-bot: I haz the power, 2015-08-13 18:40:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-13 18:40:34]

Try jobs failed on following builders:
  v8_mac_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel/builds/8653)
  v8_presubmit on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/4908)

[rossberg, 2015-08-17 09:01:13]

The CQ bit was checked by rossberg@chromium.org

[rossberg, 2015-08-17 09:01:13]

The patchset sent to the CQ was uploaded after l-g-t-m from adamk@chromium.org
Link to the patchset: https://codereview.chromium.org/1287063004/#ps40001
(title: "Rebase")

[commit-bot: I haz the power, 2015-08-17 09:01:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1287063004/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1287063004/40001

[commit-bot: I haz the power, 2015-08-17 09:11:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-17 09:11:48]

Try jobs failed on following builders:
  v8_mac_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel/builds/8701)

[rossberg, 2015-08-17 11:12:52]

The CQ bit was checked by rossberg@chromium.org

[rossberg, 2015-08-17 11:12:53]

The patchset sent to the CQ was uploaded after l-g-t-m from adamk@chromium.org
Link to the patchset: https://codereview.chromium.org/1287063004/#ps60001
(title: "Fix TODO")

[commit-bot: I haz the power, 2015-08-17 11:13:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1287063004/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1287063004/60001

[commit-bot: I haz the power, 2015-08-17 12:02:00]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-08-17 12:02:20]

Patchset 4 (id:??) landed as
https://crrev.com/6dda11ff81a08c2790f963f100d94bba4a6f28b8
Cr-Commit-Position: refs/heads/master@{#30192}

[wingo, 2015-08-24 14:10:10]

wingo@igalia.com changed reviewers:
+ wingo@igalia.com

[wingo, 2015-08-24 14:10:10]

https://codereview.chromium.org/1287063004/diff/60001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/1287063004/diff/60001/src/parser.cc#newcode3916
src/parser.cc:3916: parser_->Check(Token::ASSIGN)) {
How on earth does this work?  This function is called after the arrow function
arguments have been parsed, and its purpose is to transform the already-parsed
expression into parameters.  I can't see how this can possibly do the right
thing.

[rossberg, 2015-08-24 14:18:15]

https://codereview.chromium.org/1287063004/diff/60001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/1287063004/diff/60001/src/parser.cc#newcode3916
src/parser.cc:3916: parser_->Check(Token::ASSIGN)) {
On 2015/08/24 14:10:10, wingo wrote:
> How on earth does this work?  This function is called after the arrow function
> arguments have been parsed, and its purpose is to transform the already-parsed
> expression into parameters.  I can't see how this can possibly do the right
> thing.

You are right, it doesn't make sense. 8} Not sure what I was thinking.

Any idea why this is never hit by the tests?

[wingo, 2015-08-24 14:26:43]

On 2015/08/24 14:18:15, rossberg wrote:
> https://codereview.chromium.org/1287063004/diff/60001/src/parser.cc
> File src/parser.cc (right):
> 
>
https://codereview.chromium.org/1287063004/diff/60001/src/parser.cc#newcode3916
> src/parser.cc:3916: parser_->Check(Token::ASSIGN)) {
> On 2015/08/24 14:10:10, wingo wrote:
> > How on earth does this work?  This function is called after the arrow
function
> > arguments have been parsed, and its purpose is to transform the
already-parsed
> > expression into parameters.  I can't see how this can possibly do the right
> > thing.
> 
> You are right, it doesn't make sense. 8} Not sure what I was thinking.
> 
> Any idea why this is never hit by the tests?

:)  I would guess this is never hit because
ParseArrowFunctionFormalParameterList is only called with Token::ARROW is the
next token in the stream.  As to how it works... I guess is_simple is set to
false earlier because the expression is not a variableproxy, and the RHS is
validated as an expression already by the parser.

[rossberg, 2015-08-24 15:19:50]

https://codereview.chromium.org/1314543005