761510 - rm JSFUN_NULL_CLOSURE

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Luke Wagner [:luke]]

I don't think it serves a purpose anymore.  Also, almost all of the rest of SemanticAnalysis.cpp can be removed with this.

Comment 1

[Luke Wagner [:luke]]

Note to self: with SetFunctionKinds removed, remove PNK_UPVARS and free lexdeps eagerly.

Comment 2

[Luke Wagner [:luke]]

Attachment: rm JSFUN_NULL_CLOSURE

Step 1: remove the flag.

Comment 3

[Luke Wagner [:luke]]

Attachment: rm SetFunctionKinds and PNK_UPVARS

Mmmm, tasy.

Comment 4

[Luke Wagner [:luke]]

*tasty, even.

Comment 5

[Jim Blandy :jimb]

Comment on attachment 641654 [details] [diff] [review]
rm JSFUN_NULL_CLOSURE

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

It is a good day.

Comment 6

[Luke Wagner [:luke]]

Attachment: rm PN_NAMESET

jimb pointed out that patch 2 makes PN_NAMESET dead as well!

Comment 7

[Jim Blandy :jimb]

Comment on attachment 641681 [details] [diff] [review]
rm SetFunctionKinds and PNK_UPVARS

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

It looks to me as if PNK_UPVARS is the only user of the nameset arity; so that's a bunch more stuff that might as well go too.

::: js/src/frontend/ParseNode.h
@@ +822,5 @@
>      bool isGeneratorExpr() const {
>          if (getKind() == PNK_LP) {
>              ParseNode *callee = this->pn_head;
>              if (callee->getKind() == PNK_FUNCTION) {
> +                ParseNode *body = callee->pn_body;

If you like, I think it would be okay to just write 'callee->pn_body->getKind() == ...' here, and perhaps combine the inner if with the enclosing if. The temporary variable doesn't seem to serve any purpose any more.

Comment 8

[Luke Wagner [:luke]]

D'oh, I realized that the existing patch makes enclosing functions heavyweight if any nested function contains a free variable.  For example, in:
  function f() { (function g() { x++ })() }
f will be marked heavyweight.  This is because g creates a placeholder which is marked as 'closed' when it is hoisted into f.  However, it is too early to say whether f contains a definition of 'x' yet.

I think the solution is to mark functions as having closed variables when they have finished parsing.  This would also give us a simple place to compile the list of upvars instead of the scattered noteClosedVar/noteClosedArg stuff.

Comment 9

[Luke Wagner [:luke]]

Hrm, the solution to this will fit nicely into bug 775323, so I'll land the JSFUN_NULL_CLOSURE-removal now and roll the SetFunctionKinds-removal into bug 775323.

https://hg.mozilla.org/integration/mozilla-inbound/rev/6a96719b7acf

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/6a96719b7acf

Comment 11

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

`fun` was unused in SetFunctionKinds, so I landed

https://hg.mozilla.org/integration/mozilla-inbound/rev/4c75b59971a6

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/4c75b59971a6