1216966 - Miscellaneous js/src/frontend tidying

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Comment 1

[Jason Orendorff [:jorendorff]]

Attachment: Part 1: Splurge and use separate error messages for generator comprehensions that need more parentheses and yield expressions that need more parentheses

Comment 2

[Jason Orendorff [:jorendorff]]

Attachment: Part 2: Pointlessly tweak a comment about comprehension syntax

Comment 3

[Jason Orendorff [:jorendorff]]

(Sorry for that part 2, I made the change while reading and trying to understand code, and I still think it's a little better so I kept it.)

Comment 4

[Jason Orendorff [:jorendorff]]

Attachment: Part 3: Remove redundant method Parser::parenExprOrGeneratorComprehension

Comment 5

[Jason Orendorff [:jorendorff]]

Attachment: Part 4: Update a big comment about for-loops in Parser.cpp. Lots of pointless tweaking here, too

Comment 6

[Jason Orendorff [:jorendorff]]

Attachment: Part 5: Code organization and comments around parsing comprehensions

Comment 7

[Eric Faust [:efaust]]

Comment on attachment 8676865 [details] [diff] [review]
Part 1: Splurge and use separate error messages for generator comprehensions that need more parentheses and yield expressions that need more parentheses

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

Such opulence. Such...conspicuous consumption.

Comment 8

[Eric Faust [:efaust]]

Comment on attachment 8676867 [details] [diff] [review]
Part 2: Pointlessly tweak a comment about comprehension syntax

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

It's clearer this way. There's value, here.

Comment 9

[Eric Faust [:efaust]]

Comment on attachment 8676869 [details] [diff] [review]
Part 3: Remove redundant method Parser::parenExprOrGeneratorComprehension

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

Nice.

::: js/src/frontend/Parser.cpp
@@ +9386,5 @@
> +
> +            // Now just return something that will allow parsing to continue.
> +            // It doesn't matter what; when we reach the =>, we will rewind and
> +            // reparse the whole arrow function. See Parser::assignExpr.
> +            return handler.newNullLiteral(pos());

Aside: Man, what a hack! I hadn't read this comment before.

Comment 10

[Eric Faust [:efaust]]

Comment on attachment 8676873 [details] [diff] [review]
Part 5: Code organization and comments around parsing comprehensions

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

::: js/src/frontend/Parser.cpp
@@ +7572,5 @@
> + * TC39 decided to drop it.)
> + *
> + * Legacy generator expressions evaluate to legacy generators (using the
> + * StopIteration protocol); ES6-era generator expressions evaluate to ES6
> + * generators (using the `{done:, value:}` protocol).

This is a great comment. It's quite clear, and tells the history of the situation concisely.

@@ +8467,5 @@
>  }
>  
> +
> +/* * */
> +

Sssure. I think this comment is maybe unnecessary, but it does bracket the newly added Comprehensions comment, so it's fine.

Comment 11

[Jason Orendorff [:jorendorff]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6f659dd470d83d23024bf1bcc173614cb8a12643
Bug 1216966 - Part 1: Splurge and use separate error messages for generator comprehensions that need more parentheses and yield expressions that need more parentheses. r=efaust.

https://hg.mozilla.org/integration/mozilla-inbound/rev/30f2d69ecd7c3d9e8faa167f49adf9bd1c615fcc
Bug 1216966 - Part 2: Tweak a comment about comprehension syntax. r=efaust.

https://hg.mozilla.org/integration/mozilla-inbound/rev/1d4b1adfb5724a94ae6c82c7c211b607ee1e9a4a
Bug 1216966 - Part 3: Remove redundant method Parser::parenExprOrGeneratorComprehension. r=efaust.

https://hg.mozilla.org/integration/mozilla-inbound/rev/cd16e0e4852bbced3db97c67b849653dab26d123
Bug 1216966 - Part 4: Update a big comment about for-loops in Parser.cpp. r=efaust.

https://hg.mozilla.org/integration/mozilla-inbound/rev/bb5b95d5f621a62758a4d26f06ee15d98f2ac43f
Bug 1216966 - Part 5: Code organization and comments around parsing comprehensions. r=efaust.

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6f659dd470d8
https://hg.mozilla.org/mozilla-central/rev/30f2d69ecd7c
https://hg.mozilla.org/mozilla-central/rev/1d4b1adfb572
https://hg.mozilla.org/mozilla-central/rev/cd16e0e4852b
https://hg.mozilla.org/mozilla-central/rev/bb5b95d5f621