Closed Bug 696953 Opened 10 years ago Closed 10 years ago

Separate analyzeFunctions pass from the parser


(Core :: JavaScript Engine, defect)

Not set





(Reporter: jorendorff, Assigned: jorendorff)



(4 files, 1 obsolete file)

It's really basically not a part of the Parser at all. It hardly uses any of the Parser fields.
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.
Assignee: general → jorendorff
Attachment #569226 - Flags: review?(jwalden+bmo)
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.
Attachment #569227 - Flags: review?(jwalden+bmo)
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.
Attachment #569233 - Flags: review?(jwalden+bmo)
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.
Attachment #569238 - Flags: review?(jwalden+bmo)
Attachment #569227 - Attachment is obsolete: true
Attachment #569227 - Flags: review?(jwalden+bmo)
Attachment #569280 - Flags: review?(jwalden+bmo)
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]:

Attachment #569226 - Flags: review?(jwalden+bmo) → review+
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; }

Attachment #569280 - Flags: review?(jwalden+bmo) → review+
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?
Attachment #569233 - Flags: review?(jwalden+bmo) → review+
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?
Attachment #569238 - Flags: review?(jwalden+bmo) → review+
You need to log in before you can comment on or make changes to this bug.