The default bug view has changed. See this FAQ.

Separate analyzeFunctions pass from the parser

RESOLVED FIXED in mozilla10

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: jorendorff, Assigned: jorendorff)

Tracking

Trunk
mozilla10
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 1 obsolete attachment)

(Assignee)

Description

6 years ago
It's really basically not a part of the Parser at all. It hardly uses any of the Parser fields.
(Assignee)

Comment 1

6 years ago
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.
Assignee: general → jorendorff
Attachment #569226 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 2

6 years ago
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.
Attachment #569227 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 3

6 years ago
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.
Attachment #569233 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 4

6 years ago
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.
Attachment #569238 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 5

6 years ago
Created attachment 569280 [details] [diff] [review]
Part 2 - Move GlobalScope from BytecodeCompiler to BytecodeEmitter, v2

Fixed!
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]:
-----------------------------------------------------------------

rs=me
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; }

\o/
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+
https://hg.mozilla.org/mozilla-central/rev/00442d1ed250
https://hg.mozilla.org/mozilla-central/rev/e52de86f9511
https://hg.mozilla.org/mozilla-central/rev/0469a44abae7
https://hg.mozilla.org/mozilla-central/rev/91b0cb2f217e
https://hg.mozilla.org/mozilla-central/rev/0e49c37ede76
Status: NEW → RESOLVED
Last Resolved: 6 years ago
OS: Mac OS X → All
Hardware: x86 → All
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
Version: Other Branch → Trunk
You need to log in before you can comment on or make changes to this bug.