759246 - Clean-up SharedContext a bit

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

Attachment: 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 [inactive]]

Attachment: 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 [inactive]]

Attachment: 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 [inactive]]

Attachment: 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]]

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.

Comment 5

[Nicholas Nethercote [inactive]]

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

Comment 6

[(no longer active)]

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