750282 - Remove TCF_COMPILING

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

Attachment: patch 1: avoid definitely-succeeding asBytecodeEmitter() calls

TreeContext::asBytecodeEmitter is a down-cast.  It kinda sucks, because it exposes in an unclean way the fact that Parser::tc can be a TreeContext or a BytecodeEmitter, depending on the context.

This first patch adds BytecodeEmitter::parentBCE, which has the same value as TreeContext::parent, but a different type.  It allows us to remove the places where asBytecodeEmitter() is called where we know the down-cast will succeed.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: patch 2: avoid possibly-succeeding asBytecodeEmitter() calls

This second patch converts DefineGlobal and BindTopLevelVar to methods within TreeContext, which BytecodeEmitter overrides.  This allows us to remove the places where asBytecodeEmitter() is called where we are unsure if the down-cast will succeed.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: patch 3: remove asBytecodeEmitter and friends

This third patch removes asBytecodeEmitter(), TCF_COMPILING, and compiling().

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: new patch

Actually, the down-casts that are known to succeed (i.e. those removed in 
patch 1) aren't that bad;  no worse than adding a |parentBCE| field that has
the same value as |parent|.  

Here's a rolled-up patch that keeps those not-so-bad down-casts.
Summarizing its effects:

- It removes the can-fail down-casts by moving defineGlobal() and 
  bindTopLevelVar() into TreeContext.  

- It moves the down-cast method from TreeContext() into BytecodeEmitter(),
  which is a much better spot for it.
  
- TCF_COMPILING and compiling() are no longer necessary.
  
The main effect is that Parser doesn't need to know that its TreeContext 
might actually be a BytecodeEmitter.  This is a good thing in its own right
and a step towards making Parser.cpp module not depend on BytecodeEmitter.h.

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: newer patch

This patch is even simpler.  It applies on top of the patch in bug 751818.  Net effects:

- TCF_COMPILING and compiling() are removed.

- The TreeContext-to-BytecodeEmitter downcasts are moved.

Comment 5

[Luke Wagner [:luke]]

coool

Comment 6

[Jason Orendorff [:jorendorff]]

Comment on attachment 619781 [details] [diff] [review]
new patch

Review of attachment 619781 [details] [diff] [review]:
-----------------------------------------------------------------

r=me if what you intended is for the two methods to be virtual. Otherwise I'm confused and need to take another look.

I'm assuming the two functions you moved are pretty much unchanged, just moving them from one file to another and turning them into BytecodeEmitter methods.

::: js/src/frontend/BytecodeEmitter.h
@@ +372,5 @@
>      }
>  
>      OwnedAtomDefnMapPtr lexdeps;    /* unresolved lexical name dependencies */
>  
> +    TreeContext     *parent;        /* Enclosing function or global context.  */

Why do you think the longer comment here is no longer helpful?

@@ +484,5 @@
>          return flags & TCF_FUN_EXTENSIBLE_SCOPE;
>      }
>  
> +    bool defineGlobal(ParseNode *pn, PropertyName *name);
> +    bool bindTopLevelVar(BindData *data, ParseNode *pn);

Hmm. These are non-virtual. (more about this in two comments below)

@@ +641,5 @@
> +    // This is a down-cast.  It's necessary and safe -- although
> +    // TreeContext::parent is a |TreeContext *|, a BytecodeEmitter's parent is
> +    // always itself a BytecodeEmitter.
> +    BytecodeEmitter *parentBCE() {
> +        return static_cast<BytecodeEmitter *>(parent);

It would be cool if the parent data member was private, the base class had a parent() method, and this parent() method just hid that one. It wouldn't be necessary for either parent() to be virtual. Just a thought.

@@ +698,5 @@
>  
>      bool needsImplicitThis();
>  
> +    bool defineGlobal(ParseNode *pn, PropertyName *name);   // overrides
> +    bool bindTopLevelVar(BindData *data, ParseNode *pn);    // overrides

If these are supposed to be virtual, consider using MOZ_OVERRIDE instead of comments.

::: js/src/frontend/Parser.cpp
@@ +1703,5 @@
>          pn->pn_body = body;
>      }
>  
> +    if (!outertc->inFunction() && bodyLevel && kind == Statement) {
> +        if (!outertc->defineGlobal(pn, funName))

Because the method is nonvirtual, this will be dispatched to the base-class defineGlobal method which does nothing... right?

If so, it must be a bug. Make sure we have a test that fails here, adding one if needed.

Same goes for bindTopLevelVar.

Comment 7

[Jason Orendorff [:jorendorff]]

Comment on attachment 620977 [details] [diff] [review]
newer patch

r+ without the qualification on the previous version of the patch.

The nits about deleting comments still stand, otherwise this is fine.

Comment 8

[Nicholas Nethercote [inactive]]

I got rid of the comments mostly I have upcoming patches that will completely separate TreeContext and BytecodeEmitter, whereupon the comment will no longer be true.  I can put it back for the moment, though.

Comment 9

[Nicholas Nethercote [inactive]]

I ended up removing the comment, because I didn't want to put it back in just to remove it in a later patch.  Don't worry, things will be getting much better soon :)

https://hg.mozilla.org/integration/mozilla-inbound/rev/c6ea9609aa15

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/c6ea9609aa15