Jason Orendorff [:jorendorff] Assignee
	Description • 11 years ago

As of now this is in sec. 13-2 of the draft spec:
  http://people.mozilla.org/~jorendorff/es6-draft.html#sec-13.2

	Jason Orendorff [:jorendorff] Assignee
	Comment 2 • 11 years ago

Work to do that I can think of offhand:
- `() => x` arrow functions with no arguments don't work yet
- `(...rest) => x` arrow functions with rest arguments don't work yet
- 'this' should be lexical, unlike other functions
- arrow functions should have no 'arguments' binding, unlike other functions
- FunctionToString is moderately broken

	Jason Orendorff [:jorendorff] Assignee
	Comment 3 • 11 years ago

needs tests.

	Jason Orendorff [:jorendorff] Assignee
	Comment 4 • 11 years ago

More TODO items:
- tests for arrow-block functions
- Reflect.parse support
- ban yield in arrow functions

	Jason Orendorff [:jorendorff] Assignee
	Comment 5 • 11 years ago

fiveop, a volunteer, wants to take on the "() => body" syntax.

Just did Reflect.parse; I'll work on rest arguments next...

	Jason Orendorff [:jorendorff] Assignee
	Comment 8 • 11 years ago

OK, I'm punting lexical 'this' to follow-up bug 848062 so that this can land.

	Brian Hackett [Laid off!]
	Comment 9 • 11 years ago

Comment on attachment 721380 [details] [diff] [review]
v5

Review of attachment 721380 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/Parser.cpp
@@ +1483,4 @@
>  {
> +    FunctionBox *funbox = pc->sc->asFunctionBox();
> +
> +    bool parenFree = false;

Maybe name this 'parenFreeArrow' to be more descriptive.

@@ +1603,5 @@
>                  if (!defineArg(funcpn, name, disallowDuplicateArgs, &duplicatedArg))
>                      return false;
>  
>                  if (tokenStream.matchToken(TOK_ASSIGN)) {
> +                    JS_ASSERT(!parenFree);

What guarantees this assert?

@@ +4803,5 @@
>  #endif
>  
> +    // Save two source locations, both just in case we turn out to be in an
> +    // arrow function. 'offset' is for FunctionToString. 'start' is for
> +    // rewinding and reparsing arguments.

This seems unfortunate, can you describe a bit more in the comment why this is necessary?

@@ +4807,5 @@
> +    // rewinding and reparsing arguments.
> +    if (tokenStream.getToken() == TOK_ERROR)
> +        return null();
> +    size_t offset = tokenStream.offsetOfToken(tokenStream.currentToken());
> +    tokenStream.ungetToken();

It seems like you could just compute offset in the TOK_ARROW case of the switch, since you will be seeking back to the start of the ::assignExpr anyways.

	Jason Orendorff [:jorendorff] Assignee
	Comment 10 • 11 years ago

(In reply to Brian Hackett (:bhackett) from comment #9)
> ::: js/src/frontend/Parser.cpp
> >                  if (tokenStream.matchToken(TOK_ASSIGN)) {
> > +                    JS_ASSERT(!parenFree);
> 
> What guarantees this assert?

I added a comment:

                // A default argument without parentheses would look like:
                // a = expr => body, but both operators are right-associative, so
                // that would have been parsed as a = (expr => body) instead.
                // Therefore it's impossible to get here with parenFreeArrow.
                JS_ASSERT(!parenFreeArrow);

> > +    // rewinding and reparsing arguments.
> > +    if (tokenStream.getToken() == TOK_ERROR)
> > +        return null();
> > +    size_t offset = tokenStream.offsetOfToken(tokenStream.currentToken());
> > +    tokenStream.ungetToken();
> 
> It seems like you could just compute offset in the TOK_ARROW case of the
> switch, since you will be seeking back to the start of the ::assignExpr
> anyways.

Yup. Done.

	Nicholas Nethercote [inactive]
	Comment 11 • 11 years ago

This appears to have landed.  Please add the URL for the patch.

	Nicholas Nethercote [inactive]
	Comment 12 • 11 years ago

Comment on attachment 721380 [details] [diff] [review]
v5

Review of attachment 721380 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/TokenStream.h
@@ +558,5 @@
>      }
>  
>      TokenKind peekToken() {
>          if (lookahead != 0) {
> +            JS_ASSERT(lookahead <= maxLookahead);

Given the following line, shouldn't this be |1 <= maxLookahead|?

@@ +573,5 @@
>      }
>  
>      TokenKind peekTokenSameLine(unsigned withFlags = 0) {
> +        if (lookahead != 0) {
> +            JS_ASSERT(lookahead <= maxLookahead);

Ditto.

	Ryan VanderMeulen [:RyanVM]
	Comment 13 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/bf3ce88c6ea3

	Philipp Matthias Schäfer [:fiveop]
	Comment 14 • 11 years ago

@Comment 12: I think both asserts are unnecessary. We (should) make sure that |lookahead| stays within bounds in the ungetToken() function. And as soon as |lookahead > 0| holds, we know that in |tokens[cursor + 1]| is the token after current Token, so executing the 'then' clause is fine.

	Nicholas Nethercote [inactive]
	Comment 15 • 11 years ago

> @Comment 12: I think both asserts are unnecessary.

I agree :)  Feel free to make that change in bug 846933.