Last Comment Bug 767202 - Cleave StmtInfo in twain
: Cleave StmtInfo in twain
Status: RESOLVED FIXED
[js:t]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla16
Assigned To: Nicholas Nethercote [:njn] (on vacation until July 11)
:
Mentors:
Depends on:
Blocks: UntangleFrontEnd
  Show dependency treegraph
 
Reported: 2012-06-21 16:51 PDT by Nicholas Nethercote [:njn] (on vacation until July 11)
Modified: 2012-06-30 12:46 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
(part 1) - Don't export PopStatementBCE(). (1.64 KB, patch)
2012-06-21 18:37 PDT, Nicholas Nethercote [:njn] (on vacation until July 11)
bhackett1024: review+
Details | Diff | Review
(part 2) - Inline EnterFunction(). (1.48 KB, patch)
2012-06-21 18:42 PDT, Nicholas Nethercote [:njn] (on vacation until July 11)
bhackett1024: review+
Details | Diff | Review
(part 3) - Split StmtInfo into two. (75.29 KB, patch)
2012-06-21 18:44 PDT, Nicholas Nethercote [:njn] (on vacation until July 11)
bhackett1024: review+
Details | Diff | Review
(part 4) - Initialize bodyid in TreeContext's constructor. (5.25 KB, patch)
2012-06-21 18:45 PDT, Nicholas Nethercote [:njn] (on vacation until July 11)
bhackett1024: review+
Details | Diff | Review
bundle for gavin (8.98 KB, application/octet-stream)
2012-06-28 17:36 PDT, Nicholas Nethercote [:njn] (on vacation until July 11)
no flags Details

Description Nicholas Nethercote [:njn] (on vacation until July 11) 2012-06-21 16:51:24 PDT
StmtInfo is used in quite different ways by Parser and BytecodeEmitter.  We can improve things by splitting it in two.
Comment 1 Nicholas Nethercote [:njn] (on vacation until July 11) 2012-06-21 18:37:51 PDT
Created attachment 635565 [details] [diff] [review]
(part 1) - Don't export PopStatementBCE().

PopStatementBCE() doesn't need to be exported.
Comment 2 Nicholas Nethercote [:njn] (on vacation until July 11) 2012-06-21 18:42:17 PDT
Created attachment 635567 [details] [diff] [review]
(part 2) - Inline EnterFunction().

This patch inlines EnterFunction at its two call sites, in preparation for
the next patch.
Comment 3 Nicholas Nethercote [:njn] (on vacation until July 11) 2012-06-21 18:44:21 PDT
Created attachment 635568 [details] [diff] [review]
(part 3) - Split StmtInfo into two.

StmtInfo is used in Parser and BytecodeEmitter.  But Parser never uses the
|update|, |breaks| and |continues| fields, and BytecodeEmitter never uses
the the |blockid| field.

This patch splits StmtInfo into StmtInfoTC and StmtInfoBCE.  StmtInfoBase
holds the common parts.  (I recommend reading the changes to TreeContext.h
first.)  

It uses templates for several functions that operate on both types, e.g.
PushStatement().  (I had to move the definitions of these functions from
TreeContext.cpp to TreeContext-inl.h for the templates to work.)

The patch moves SharedContext::{topStmt,topScopeStmt} into both TreeContext
and BytecodeEmitter, because those fields now have different types in the
two cases.  It also moves SharedContext::blockChain because it's closely
associated with the {topStmt,topScopeStmt} fields;  this change isn't
strictly necessary.  For all three fields this works because they weren't
actually shared by Parser and BytecodeEmitter, even though they were present
in SharedContext.  (I have a long-term goal of making SharedContext as small
as possible.)

Finally, the patch also moves SharedContext::{bodyid,blockidGen} into
TreeContext;  this was necessary because StmtInfoBCE doesn't have the
|blockid| field.
Comment 4 Nicholas Nethercote [:njn] (on vacation until July 11) 2012-06-21 18:45:43 PDT
Created attachment 635569 [details] [diff] [review]
(part 4) - Initialize bodyid in TreeContext's constructor.

This patch initializes TreeContext::bodyid in TreeContext's constructor, and
then increments TreeContext::blockidGen in TreeContext::init().  This lets
us avoid numerous explicit calls to GenerateBlockId().
Comment 5 :Ms2ger 2012-06-23 07:53:23 PDT
Comment on attachment 635567 [details] [diff] [review]
(part 2) - Inline EnterFunction().

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

::: js/src/frontend/Parser.cpp
@@ +5453,5 @@
>              return NULL;
>  
> +        gensc.blockidGen = outertc->sc->blockidGen;
> +        if (!GenerateBlockId(&gensc, gensc.bodyid))
> +            return false;

I don't think false is what you want.
Comment 6 Nicholas Nethercote [:njn] (on vacation until July 11) 2012-06-23 15:08:16 PDT
> I don't think false is what you want.

Thanks.  It's surprising that GCC doesn't warn about that.  I've fixed several false/NULL mismatches recently, hopefully more than I've introduced :)
Comment 7 Nicholas Nethercote [:njn] (on vacation until July 11) 2012-06-28 17:36:01 PDT
Created attachment 637738 [details]
bundle for gavin

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