764714 - Three small BytecodeEmitter clean-ups

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Only set JSScript::savedCallerFun when appropriate.

In bug 758509 comment 14 I realized that if a script, S, has
savedCallerFun==true, then any nested functions within it also will.  But
that's not right, because it implies that the nested function will have
objects[0] set to the function enclosing S.

This patch changes things so that savedCallerFun==false for any nested
functions.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Move Parser::callerFrame to BytecodeEmitter.

Parser::callerFrame is never used by the Parser;  it's only used by
BytecodeEmitter, via bce->parser->callerFrame.  This patch moves it to
BytecodeEmitter::callerFrame.  This required inlining
Parser::checkForArgumentsAndRest() at its single call site, and adding an
argument to AnalyzeFunctions().

It's an improvement because the Parser no longer sees it, and it reduces
BytecodeEmitter's dependency on Parser.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - Remove BytecodeEmitter::GlobalScope.

BytecodeEmitter::globalScope has a single use, and it can be replaced with a
boolean.  This patch does that, and initializes it via the constructor,
allowing the bool field to be const.

While I was in there, I also initialized BytecodeEmitter::parent via the
constructor, allowing it to be made const as well.  And I made
BytecodeEmitter::sc const, too.

Comment 3

[:Benjamin Peterson]

Comment on attachment 633041 [details] [diff] [review]
(part 2) - Move Parser::callerFrame to BytecodeEmitter.

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

::: js/src/frontend/BytecodeCompiler.cpp
@@ +228,5 @@
>          return NULL;
>      }
>  #endif
>  
> +    // It's an error if |arguments| or |rest| is present outside a function.

This is actually checking to see if someone is trying to get |arguments| of a function with a rest parameter like this:

function f(...rest) {
    a = eval("arguments");
}

Comment 4

[Jason Orendorff [:jorendorff]]

Comment on attachment 633041 [details] [diff] [review]
(part 2) - Move Parser::callerFrame to BytecodeEmitter.

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

Nice!

::: js/src/frontend/BytecodeCompiler.cpp
@@ +228,5 @@
>          return NULL;
>      }
>  #endif
>  
> +    // It's an error if |arguments| or |rest| is present outside a function.

The comment should say
    // It's an error to use |arguments| in a function that has a rest parameter.

Comment 5

[Jason Orendorff [:jorendorff]]

Comment on attachment 633042 [details] [diff] [review]
(part 3) - Remove BytecodeEmitter::GlobalScope.

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

::: js/src/frontend/BytecodeEmitter.h
@@ +71,2 @@
>  
> +    RootedScript    script;         /* the JSScript we're ultimately producing */

Waldo mentioned preferring the full form Rooted<JSScript*> to the typedef, and I guess the consensus is in agreement; consider changing it back?

Comment 6

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/23015006c975
https://hg.mozilla.org/integration/mozilla-inbound/rev/24ea9cbc30b8
https://hg.mozilla.org/integration/mozilla-inbound/rev/1ac44e8c1ede

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/23015006c975
https://hg.mozilla.org/mozilla-central/rev/24ea9cbc30b8
https://hg.mozilla.org/mozilla-central/rev/1ac44e8c1ede