The default bug view has changed. See this FAQ.

Three small BytecodeEmitter clean-ups

RESOLVED FIXED in mozilla16

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

unspecified
mozilla16
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [js:t])

Attachments

(3 attachments)

(Assignee)

Description

5 years ago
Created attachment 633040 [details] [diff] [review]
(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.
Attachment #633040 - Flags: review?(jorendorff)
(Assignee)

Comment 1

5 years ago
Created attachment 633041 [details] [diff] [review]
(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.
Attachment #633041 - Flags: review?(jorendorff)
(Assignee)

Comment 2

5 years ago
Created attachment 633042 [details] [diff] [review]
(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.
Attachment #633042 - Flags: review?(jorendorff)

Comment 3

5 years ago
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");
}
Attachment #633040 - Flags: review?(jorendorff) → review+
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.
Attachment #633041 - Flags: review?(jorendorff) → review+
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?
Attachment #633042 - Flags: review?(jorendorff) → review+
(Assignee)

Comment 6

5 years ago
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
https://hg.mozilla.org/mozilla-central/rev/23015006c975
https://hg.mozilla.org/mozilla-central/rev/24ea9cbc30b8
https://hg.mozilla.org/mozilla-central/rev/1ac44e8c1ede
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
You need to log in before you can comment on or make changes to this bug.