Closed Bug 767202 Opened 9 years ago Closed 9 years ago

Cleave StmtInfo in twain

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla16

People

(Reporter: n.nethercote, Assigned: n.nethercote)

References

Details

(Whiteboard: [js:t])

Attachments

(5 files)

StmtInfo is used in quite different ways by Parser and BytecodeEmitter.  We can improve things by splitting it in two.
PopStatementBCE() doesn't need to be exported.
Attachment #635565 - Flags: review?(bhackett1024)
This patch inlines EnterFunction at its two call sites, in preparation for
the next patch.
Attachment #635567 - Flags: review?(bhackett1024)
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.
Attachment #635568 - Flags: review?(bhackett1024)
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().
Attachment #635569 - Flags: review?(bhackett1024)
Attachment #635565 - Flags: review?(bhackett1024) → review+
Attachment #635567 - Flags: review?(bhackett1024) → review+
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.
> 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 :)
Attachment #635568 - Flags: review?(bhackett1024) → review+
Attachment #635569 - Flags: review?(bhackett1024) → review+
Attached file bundle for gavin
Attachment #637738 - Attachment mime type: text/plain → application/octet-stream
You need to log in before you can comment on or make changes to this bug.