Last Comment Bug 750282 - Remove TCF_COMPILING
: Remove TCF_COMPILING
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla15
Assigned To: Nicholas Nethercote [:njn]
:
Mentors:
Depends on: 751818
Blocks: UntangleFrontEnd 750580
  Show dependency treegraph
 
Reported: 2012-04-30 08:20 PDT by Nicholas Nethercote [:njn]
Modified: 2012-05-10 08:04 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch 1: avoid definitely-succeeding asBytecodeEmitter() calls (5.35 KB, patch)
2012-04-30 08:20 PDT, Nicholas Nethercote [:njn]
no flags Details | Diff | Splinter Review
patch 2: avoid possibly-succeeding asBytecodeEmitter() calls (16.23 KB, patch)
2012-04-30 08:23 PDT, Nicholas Nethercote [:njn]
no flags Details | Diff | Splinter Review
patch 3: remove asBytecodeEmitter and friends (14.61 KB, patch)
2012-04-30 08:24 PDT, Nicholas Nethercote [:njn]
no flags Details | Diff | Splinter Review
new patch (33.77 KB, patch)
2012-04-30 17:29 PDT, Nicholas Nethercote [:njn]
no flags Details | Diff | Splinter Review
newer patch (17.59 KB, patch)
2012-05-03 23:41 PDT, Nicholas Nethercote [:njn]
jorendorff: review+
Details | Diff | Splinter Review

Description Nicholas Nethercote [:njn] 2012-04-30 08:20:42 PDT
Created attachment 619572 [details] [diff] [review]
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 [:njn] 2012-04-30 08:23:11 PDT
Created attachment 619574 [details] [diff] [review]
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 [:njn] 2012-04-30 08:24:24 PDT
Created attachment 619576 [details] [diff] [review]
patch 3: remove asBytecodeEmitter and friends

This third patch removes asBytecodeEmitter(), TCF_COMPILING, and compiling().
Comment 3 Nicholas Nethercote [:njn] 2012-04-30 17:29:22 PDT
Created attachment 619781 [details] [diff] [review]
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 [:njn] 2012-05-03 23:41:07 PDT
Created attachment 620977 [details] [diff] [review]
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] 2012-05-04 09:52:32 PDT
coool
Comment 6 Jason Orendorff [:jorendorff] 2012-05-04 10:42:16 PDT
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] 2012-05-04 10:56:53 PDT
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 [:njn] 2012-05-04 14:29:44 PDT
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 [:njn] 2012-05-09 17:47:00 PDT
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] 2012-05-10 08:04:32 PDT
https://hg.mozilla.org/mozilla-central/rev/c6ea9609aa15

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