Closed
Bug 759246
Opened 12 years ago
Closed 12 years ago
Clean-up SharedContext a bit
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla15
People
(Reporter: n.nethercote, Assigned: n.nethercote)
References
Details
(Whiteboard: [js:t])
Attachments
(4 files)
4.21 KB,
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
7.26 KB,
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
45.10 KB,
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
18.48 KB,
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
This preliminary patch just uses |&sc| directly in a couple of places instead of the less direct |bce.sc|.
Attachment #627848 -
Flags: review?(jorendorff)
Assignee | ||
Comment 1•12 years ago
|
||
SharedContext holds things that must be seen by both the Parser and the BytecodeEmitter. SharedContext::functionList is only needed by the Parser, so it can be moved into TreeContext, which is only seen by the Parser.
Attachment #627849 -
Flags: review?(jorendorff)
Assignee | ||
Comment 2•12 years ago
|
||
SharedContext is partially initialized in its constructor and partly afterwards. This patch moves the initialization of {fun_,funbox_,scopeChain_} to the constructor; this allows them to be made const. Once that's done, it's clear that |inFunction| is redundant w.r.t. them. Also, the distinction between a SharedContext that applies to global code and one that applies to function code is now a bit stronger, e.g. thanks to various assertions. (I even tried creating GlobalSharedContext and FunctionSharedContext sub-classes, but it was more trouble than it was worth.) BTW, should the new arguments to SharedContext's constructor be Handles?
Attachment #627850 -
Flags: review?(jorendorff)
Assignee | ||
Comment 3•12 years ago
|
||
SharedContext::inForInit is used by both Parser and BCE, but the uses by these two classes are entirely independent. So this patch removes it from SharedContext and puts one each in Parser and BytecodeEmitter.
Attachment #627851 -
Flags: review?(jorendorff)
Updated•12 years ago
|
Whiteboard: [js:t]
Updated•12 years ago
|
Attachment #627848 -
Flags: review?(jorendorff) → review+
Updated•12 years ago
|
Attachment #627849 -
Flags: review?(jorendorff) → review+
Comment 4•12 years ago
|
||
Comment on attachment 627850 [details] [diff] [review] Patch 3: initialize more SharedContext stuff via the constructor Review of attachment 627850 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/frontend/BytecodeEmitter.cpp @@ +1563,5 @@ > { > if (!parser->compileAndGo) > return true; > + > + if (!sc->inFunction()) { Nit: Maybe switch the two arms of this if/else, so it can say "if (sc->inFunction())"? ::: js/src/frontend/TreeContext.h @@ +167,5 @@ > + // In theory, |fun*| flags are only relevant if |inFunction()| is true. > + // However, we get and set in some cases where |inFunction()| is false, > + // which is why |INFUNC| doesn't appear in all of the fun* and setFun* > + // functions below. > + #define INFUNC JS_ASSERT(inFunction()) Nit: #undef INFUNC when you're done with it. Also, prevailing style seems to be to put the # in the first column no matter what else is going on.
Attachment #627850 -
Flags: review?(jorendorff) → review+
Updated•12 years ago
|
Attachment #627851 -
Flags: review?(jorendorff) → review+
Assignee | ||
Comment 5•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/645aaab2d9e6 https://hg.mozilla.org/integration/mozilla-inbound/rev/be82d1d15eea https://hg.mozilla.org/integration/mozilla-inbound/rev/4e67ee6e338c https://hg.mozilla.org/integration/mozilla-inbound/rev/81821baba288
Target Milestone: --- → mozilla15
Comment 6•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/645aaab2d9e6 https://hg.mozilla.org/mozilla-central/rev/be82d1d15eea https://hg.mozilla.org/mozilla-central/rev/4e67ee6e338c https://hg.mozilla.org/mozilla-central/rev/81821baba288
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•