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

(part 1) - Don't export PopStatementBCE(). (1.64 KB, patch)
2012-06-21 18:37 PDT, Nicholas Nethercote [:njn]
bhackett1024: review+
Details | Diff | Review
(part 2) - Inline EnterFunction(). (1.48 KB, patch)
2012-06-21 18:42 PDT, Nicholas Nethercote [:njn]
bhackett1024: review+
Details | Diff | Review
(part 3) - Split StmtInfo into two. (75.29 KB, patch)
2012-06-21 18:44 PDT, Nicholas Nethercote [:njn]
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]
bhackett1024: review+
Details | Diff | Review
bundle for gavin (8.98 KB, application/octet-stream)
2012-06-28 17:36 PDT, Nicholas Nethercote [:njn]
no flags Details

Description Nicholas Nethercote [:njn] 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] 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] 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] 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

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] 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] 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] 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.