Last Comment Bug 764714 - Three small BytecodeEmitter clean-ups
: Three small BytecodeEmitter clean-ups
Status: RESOLVED FIXED
[js:t]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla16
Assigned To: Nicholas Nethercote [:njn]
:
Mentors:
Depends on: 758509
Blocks: UntangleFrontEnd
  Show dependency treegraph
 
Reported: 2012-06-13 23:27 PDT by Nicholas Nethercote [:njn]
Modified: 2012-06-22 03:44 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
(part 1) - Only set JSScript::savedCallerFun when appropriate. (1.97 KB, patch)
2012-06-13 23:27 PDT, Nicholas Nethercote [:njn]
jorendorff: review+
Details | Diff | Splinter Review
(part 2) - Move Parser::callerFrame to BytecodeEmitter. (21.92 KB, patch)
2012-06-13 23:28 PDT, Nicholas Nethercote [:njn]
jorendorff: review+
Details | Diff | Splinter Review
(part 3) - Remove BytecodeEmitter::GlobalScope. (9.64 KB, patch)
2012-06-13 23:29 PDT, Nicholas Nethercote [:njn]
jorendorff: review+
Details | Diff | Splinter Review

Description Nicholas Nethercote [:njn] 2012-06-13 23:27:53 PDT
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.
Comment 1 Nicholas Nethercote [:njn] 2012-06-13 23:28:55 PDT
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.
Comment 2 Nicholas Nethercote [:njn] 2012-06-13 23:29:53 PDT
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.
Comment 3 :Benjamin Peterson 2012-06-16 13:23:16 PDT
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] 2012-06-21 12:16:03 PDT
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] 2012-06-21 13:07:42 PDT
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?

Note You need to log in before you can comment on or make changes to this bug.