Last Comment Bug 759246 - Clean-up SharedContext a bit
: Clean-up SharedContext a bit
Status: RESOLVED FIXED
[js:t]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla15
Assigned To: Nicholas Nethercote [:njn]
:
Mentors:
Depends on:
Blocks: UntangleFrontEnd
  Show dependency treegraph
 
Reported: 2012-05-28 21:59 PDT by Nicholas Nethercote [:njn]
Modified: 2012-06-04 09:26 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch 1: replace bce.sc with &sc where suitable (4.21 KB, patch)
2012-05-28 21:59 PDT, Nicholas Nethercote [:njn]
jorendorff: review+
Details | Diff | Review
Patch 2: move SharedContext::functionList to TreeContext (7.26 KB, patch)
2012-05-28 22:00 PDT, Nicholas Nethercote [:njn]
jorendorff: review+
Details | Diff | Review
Patch 3: initialize more SharedContext stuff via the constructor (45.10 KB, patch)
2012-05-28 22:01 PDT, Nicholas Nethercote [:njn]
jorendorff: review+
Details | Diff | Review
Patch 4: don't share inForInit between Parser and BCE (18.48 KB, patch)
2012-05-28 22:01 PDT, Nicholas Nethercote [:njn]
jorendorff: review+
Details | Diff | Review

Description Nicholas Nethercote [:njn] 2012-05-28 21:59:11 PDT
Created attachment 627848 [details] [diff] [review]
Patch 1: replace bce.sc with &sc where suitable

This preliminary patch just uses |&sc| directly in a couple of places
instead of the less direct |bce.sc|.
Comment 1 Nicholas Nethercote [:njn] 2012-05-28 22:00:10 PDT
Created attachment 627849 [details] [diff] [review]
Patch 2: move SharedContext::functionList to TreeContext

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.
Comment 2 Nicholas Nethercote [:njn] 2012-05-28 22:01:02 PDT
Created attachment 627850 [details] [diff] [review]
Patch 3: initialize more SharedContext stuff via the constructor

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?
Comment 3 Nicholas Nethercote [:njn] 2012-05-28 22:01:51 PDT
Created attachment 627851 [details] [diff] [review]
Patch 4: don't share inForInit between Parser and BCE

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.
Comment 4 Jason Orendorff [:jorendorff] 2012-06-01 16:26:06 PDT
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.

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