Last Comment Bug 696953 - Separate analyzeFunctions pass from the parser
: Separate analyzeFunctions pass from the parser
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla10
Assigned To: Jason Orendorff [:jorendorff]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-10-24 16:40 PDT by Jason Orendorff [:jorendorff]
Modified: 2011-10-28 04:28 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part 1 - Split Parser::analyzeFunctions off into its own file, v1 (262.82 KB, patch)
2011-10-24 16:42 PDT, Jason Orendorff [:jorendorff]
jwalden+bmo: review+
Details | Diff | Splinter Review
Part 2 - Move GlobalScope from BytecodeCompiler to BytecodeEmitter, v1 (6.92 KB, patch)
2011-10-24 16:45 PDT, Jason Orendorff [:jorendorff]
no flags Details | Diff | Splinter Review
Part 3 - Remove class BytecodeCompiler, v1 (37.90 KB, patch)
2011-10-24 16:57 PDT, Jason Orendorff [:jorendorff]
jwalden+bmo: review+
Details | Diff | Splinter Review
Part 4 - Remove analyzeFunctions() and friends from class js::Parser (16.55 KB, patch)
2011-10-24 17:11 PDT, Jason Orendorff [:jorendorff]
jwalden+bmo: review+
Details | Diff | Splinter Review
Part 2 - Move GlobalScope from BytecodeCompiler to BytecodeEmitter, v2 (7.63 KB, patch)
2011-10-24 21:30 PDT, Jason Orendorff [:jorendorff]
jwalden+bmo: review+
Details | Diff | Splinter Review

Description Jason Orendorff [:jorendorff] 2011-10-24 16:40:09 PDT
It's really basically not a part of the Parser at all. It hardly uses any of the Parser fields.
Comment 1 Jason Orendorff [:jorendorff] 2011-10-24 16:42:13 PDT
Created attachment 569226 [details] [diff] [review]
Part 1 - Split Parser::analyzeFunctions off into its own file, v1

I'm calling the new files SemanticAnalysis.{h,cpp}. My hope is that I'll be able to move name binding in there too. But they can't be a single pass since analyzeFunctions requires names to be bound already. So feel free to insist on a more specific filename; it's easily done.
Comment 2 Jason Orendorff [:jorendorff] 2011-10-24 16:45:40 PDT
Created attachment 569227 [details] [diff] [review]
Part 2 - Move GlobalScope from BytecodeCompiler to BytecodeEmitter, v1

The places where we use the GlobalScope, we always have a BytecodeEmitter handy... but to get the GlobalScope, we call this compiler() method that does a horrible_cast. Blyecch.
Comment 3 Jason Orendorff [:jorendorff] 2011-10-24 16:57:08 PDT
Created attachment 569233 [details] [diff] [review]
Part 3 - Remove class BytecodeCompiler, v1

OK, so now that I'm thinking about it, this isn't exactly germane to separating analyzeFunctions from the parser. Still, we have this extra class now that GlobalScope has moved, and it's not doing anything.

This is a good thing! It's good not to have a hundred fields of state to carry around through all the phases of compilation. If the data handed off from one phase to another can be *just* the parse tree, so much the better.
Comment 4 Jason Orendorff [:jorendorff] 2011-10-24 17:11:52 PDT
Created attachment 569238 [details] [diff] [review]
Part 4 - Remove analyzeFunctions() and friends from class js::Parser

They only use a tiny handful of Parser fields. It's nice how all this worked out.

However, I broke something; :-P I have a test segfaulting because a bce is unexpectedly null. I'll hunt it down as soon as I get a chance, hopefully tonight.
Comment 5 Jason Orendorff [:jorendorff] 2011-10-24 21:30:40 PDT
Created attachment 569280 [details] [diff] [review]
Part 2 - Move GlobalScope from BytecodeCompiler to BytecodeEmitter, v2

Fixed!
Comment 6 Jeff Walden [:Waldo] (remove +bmo to email) 2011-10-26 14:20:51 PDT
Comment on attachment 569226 [details] [diff] [review]
Part 1 - Split Parser::analyzeFunctions off into its own file, v1

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

rs=me
Comment 7 Jeff Walden [:Waldo] (remove +bmo to email) 2011-10-26 14:23:04 PDT
Comment on attachment 569280 [details] [diff] [review]
Part 2 - Move GlobalScope from BytecodeCompiler to BytecodeEmitter, v2

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

::: js/src/frontend/BytecodeEmitter.h
@@ -434,5 @@
>       */
>      int sharpSlotBase;
>      bool ensureSharpSlots();
>  
> -    BytecodeCompiler *compiler() { return (BytecodeCompiler *) parser; }

\o/
Comment 8 Jeff Walden [:Waldo] (remove +bmo to email) 2011-10-26 14:37:13 PDT
Comment on attachment 569233 [details] [diff] [review]
Part 3 - Remove class BytecodeCompiler, v1

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

::: js/src/frontend/BytecodeCompiler.cpp
@@ +388,2 @@
>  
> +    if (!parser.init(chars, length, filename, lineno, version))

Kill the blank line.

::: js/src/frontend/BytecodeEmitter.h
@@ +199,5 @@
>   * BytecodeEmitter to JSScript::NewScriptFromEmitter, from script_compile_sub
>   * and any kindred functions that need to make mutable scripts (even empty
>   * ones; i.e., they can't share the const JSScript::emptyScript() singleton).
>   */
>  #define TCF_NEED_MUTABLE_SCRIPT 0x20000

Hm, this #define isn't even meaningful any more.  It gets passed places, but the only place that ever queries it does so in an assertion, and only to check that only certain bits are set in a bitset.  And the shared empty script stuff it mentions is long gone.  Remove it?
Comment 9 Jeff Walden [:Waldo] (remove +bmo to email) 2011-10-26 14:43:00 PDT
Comment on attachment 569238 [details] [diff] [review]
Part 4 - Remove analyzeFunctions() and friends from class js::Parser

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

::: js/src/frontend/SemanticAnalysis.h
@@ +18,5 @@
> + * March 31, 1998.
> + *
> + * The Initial Developer of the Original Code is
> + * Netscape Communications Corporation.
> + * Portions created by the Initial Developer are Copyright (C) 1998

Fix this.

@@ +49,5 @@
> +
> +/*
> + * Analyze the tree of functions nested within a single compilation unit,
> + * starting at tc->functionList, recursively walking its kids, then following
> + * its siblings, their kids, etc.

Could you elaborate here on what "analyze" actually means?

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