Closed Bug 753657 Opened 8 years ago Closed 8 years ago

Remove the remaining flags from TreeContextFlags

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla15

People

(Reporter: njn, Assigned: njn)

References

Details

Attachments

(3 files, 1 obsolete file)

In this bug I will remove the remaining eight flags from TreeContextFlags.
All the remaining TCF_* flags are in TCF_FUN_FLAGS, which means we can get rid
of TCF_FUN_FLAGS.

Some of the places where TCF_FUN_FLAGS were used can be further simplified
by observing that all the flags in TCF_FUN_FLAGS are set but never cleared.
This means that in functionBody(), condExpr1() and bracketedExpr(), we don't
need |oldflags|.  In other words this is a no-op:

    uint32_t oldflags = tc->sc->flags;
    ...
    tc->sc->flags = oldflags | tc->sc->flags;

(I checked this with assertions before modifying the code, just to be safe.)
Attachment #622635 - Flags: review?(luke)
TCF_STRICT_MODE presents an annoying wrinkle.  In EnterFunction, we call
newFunctionBox, and funbox->tcflags inherits TCF_STRICT_MODE_CODE from the
parent tc->sc->flags.  Back in EnterFunction, we then propagate that bit
from funbox->tcflags into the funtc->sc->flags (via a full assignment, but
TCF_STRICT_MODE_CODE is the only bit that can be set in funbox->tcflags).

This is obtuse.  This patch changes things so that:

- funbox->tcflags isn't set until LeaveFunction;

- funtc->sc->flags inherits TCF_STRICT_MODE_CODE directly from tc->sc->flags.
Attachment #622637 - Flags: review?(luke)
This replaces all the remaining TCF_* flags with fields in SharedContext and
FunctionBox.  Notable things:

- I did some slight renaming so that all flags that apply only to functions
  (six of them) start with "fun".

- I removed the existing getters/setters -- bindingsAccessedDynamically(), 
  noteBindingsAccessedDynamically(), etc.  They seem less necessary than
  they used to be, and direct field access is consistent with the other
  flags I've been creating.

- I added some assertions in NewScriptFromEmitter relating to flags that
  should/shouldn't be set for |inFunction| scripts.

- JSOPTION_STRICT_MODE is now consulted in SharedContext's constructor,
  rather than TreeContext::init().
Attachment #622638 - Flags: review?(luke)
Attachment #622635 - Flags: review?(luke) → review+
Attachment #622637 - Flags: review?(luke) → review+
For patch 3, I was wondering: what if we did something like:

struct FunctionFlags {
  // nice comments
  bool funIsHeavyweight : 1;
  ...
};

and then embed FunctionFlags in both SharedContext and FunctionBox?  The main benefits I see:
 1. single definition of fields
 2. no manual per-member copy to and from SharedContext/FunctionBox

Also, this would encourage further abstraction like private fields with memfuns that assert/enforce invariants.
I considered and rejected that because we'd have a lot of things like this:

  if (bce->sc->flags->inStrictMode())

instead of this:

  if (bce->sc->inStrictMode)

but I can do it if you feel strongly.  I guess I could put the getters/setters on SharedContext to get this:

  if (bce->sc->inStrictMode())
Alternatively, you could have BytecodeEmitter derive FunctionFlags.  (Goofy, but symmetric with BytecodeEmitter deriving TreeContext.)
New version.  Changes:

- I added ContextFlags, which encapsulates the eight flags.  I didn't call it
  FunctionFlags because it can be used with global (i.e. non-Function) code.
  Both SharedContext and FunctionBox have a ContextFlags.
  
- SharedContext and FunctionBox provide getters/setters as necessary;  
  SharedContext has them for all flags, FunctionBox for just a few.
  I used "set" as the setter prefix instead of "note", because "set" seems 
  to be more standard (e.g. the existing setStrictMode()).
Attachment #622638 - Attachment is obsolete: true
Attachment #622638 - Flags: review?(luke)
Attachment #622988 - Flags: review?(luke)
Comment on attachment 622988 [details] [diff] [review]
Patch 3, v2: remove the remaining TCF_* flags

>@@ -5457,22 +5458,30 @@ Parser::generatorExpr(ParseNode *kid)
>-        gensc.flags |= TCF_FUN_IS_GENERATOR | outertc->sc->flags;
>+        if (outertc->sc->inStrictMode())                gensc.setInStrictMode();
>+        if (outertc->sc->bindingsAccessedDynamically()) gensc.setBindingsAccessedDynamically();
>+        if (outertc->sc->funIsHeavyweight())            gensc.setFunIsHeavyweight();
>+                                                        gensc.setFunIsGenerator();
>+        if (outertc->sc->funMightAliasLocals())         gensc.setFunMightAliasLocals();
>+        if (outertc->sc->funHasExtensibleScope())       gensc.setFunHasExtensibleScope();
>+        if (outertc->sc->funArgumentsHasLocalBinding()) gensc.setFunArgumentsHasLocalBinding();
>+        if (outertc->sc->funDefinitelyNeedsArgsObj())   gensc.setFunDefinitelyNeedsArgsObj();

Can this be gensc.cxFlags = outertc->sc->cxFlags?  Otherwise, it is likely anyone adding new flags will forget to add them here (and adding them here seems to be the conservative default).  After the copy there can be the fixup which does setFunIsGenerator() etc.
Attachment #622988 - Flags: review?(luke) → review+
> Can this be gensc.cxFlags = outertc->sc->cxFlags?  Otherwise, it is likely
> anyone adding new flags will forget to add them here (and adding them here
> seems to be the conservative default).  After the copy there can be the
> fixup which does setFunIsGenerator() etc.

I don't think it can be, unfortunately -- you don't want to overwrite any of the already-set flags with |false|.

However, in practice I think at this point inStrictMode is the only one that can be set, but knowing that requires understanding the preceding code.  If I was going to rely on that I'd want to have assertions like:

  JS_ASSERT(!gensc.bindingsAccessedDynamically())
  JS_ASSERT(!getsc.funIsHeavyweight())
  ...

which is just as verbose and also prone to being overlooked if a new flag is added.

I'll take another look and see if any other options occur to me.
Blocks: 754179
Ah, good point; I assumed gensc was fresh.  Asserting only strictness is set would be good if its true.  If that isn't possible, could we avoid the future hazard (when adding a new flag) by adding an explicit ContextFlags::propgateFlagsToGenExpr(ContextFlags *dst) method to ContextFlags?
https://hg.mozilla.org/integration/mozilla-inbound/rev/d5a02a324330
https://hg.mozilla.org/integration/mozilla-inbound/rev/e1acc0dd12a8
https://hg.mozilla.org/integration/mozilla-inbound/rev/2cde430809e3

> Ah, good point; I assumed gensc was fresh.  Asserting only strictness is set
> would be good if its true.  If that isn't possible, could we avoid the
> future hazard (when adding a new flag) by adding an explicit
> ContextFlags::propgateFlagsToGenExpr(ContextFlags *dst) method to
> ContextFlags?

I ended up tweaking EnterFunction -- moving the isInStrict setting out of it -- so that gensc was fresh in this instance.  I think that makes things better.
You need to log in before you can comment on or make changes to this bug.