Clean-up SharedContext a bit

RESOLVED FIXED in mozilla15

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

unspecified
mozilla15
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [js:t])

Attachments

(4 attachments)

(Assignee)

Description

5 years ago
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|.
Attachment #627848 - Flags: review?(jorendorff)
(Assignee)

Comment 1

5 years ago
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.
Attachment #627849 - Flags: review?(jorendorff)
(Assignee)

Comment 2

5 years ago
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?
Attachment #627850 - Flags: review?(jorendorff)
(Assignee)

Comment 3

5 years ago
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.
Attachment #627851 - Flags: review?(jorendorff)
Whiteboard: [js:t]
Attachment #627848 - Flags: review?(jorendorff) → review+
Attachment #627849 - Flags: review?(jorendorff) → review+
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+
Attachment #627851 - Flags: review?(jorendorff) → review+
(Assignee)

Comment 5

5 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
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
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.