Last Comment Bug 696108 - extract EmitFor
: extract EmitFor
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla10
Assigned To: Luke Wagner [:luke]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-10-20 08:55 PDT by Luke Wagner [:luke]
Modified: 2011-10-22 06:07 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
hoist for (22.54 KB, patch)
2011-10-20 08:55 PDT, Luke Wagner [:luke]
no flags Details | Diff | Splinter Review
simplify let expr in for init (2.85 KB, patch)
2011-10-20 08:55 PDT, Luke Wagner [:luke]
no flags Details | Diff | Splinter Review
simplify let expr in for init (with reftest fix) (3.54 KB, patch)
2011-10-20 09:19 PDT, Luke Wagner [:luke]
no flags Details | Diff | Splinter Review
hoist EmitFor, split into EmitForIn and EmitNormalFor (22.11 KB, patch)
2011-10-20 15:46 PDT, Luke Wagner [:luke]
jorendorff: review+
Details | Diff | Splinter Review

Description Luke Wagner [:luke] 2011-10-20 08:55:01 PDT
Created attachment 568408 [details] [diff] [review]
hoist for

Two simple patches: 
 1. extract the TOK_FOR case in EmitTree into EmitFor and pushes declarations down to uses
 2. simplify the handling of 'for (let (x=expr) expr; ...; ...)'
Comment 1 Luke Wagner [:luke] 2011-10-20 08:55:37 PDT
Created attachment 568409 [details] [diff] [review]
simplify let expr in for init
Comment 2 Luke Wagner [:luke] 2011-10-20 09:19:54 PDT
Created attachment 568415 [details] [diff] [review]
simplify let expr in for init (with reftest fix)

A reftest caught a change in  the SyntaxError message produced.
Comment 3 Luke Wagner [:luke] 2011-10-20 10:11:02 PDT
Comment on attachment 568415 [details] [diff] [review]
simplify let expr in for init (with reftest fix)

Actually, this does change behavior so nevermind.  I'm little new here, please excuse me :)
Comment 4 Luke Wagner [:luke] 2011-10-20 15:46:30 PDT
Created attachment 568547 [details] [diff] [review]
hoist EmitFor, split into EmitForIn and EmitNormalFor

Actually, there is practically zero commonality to the for-in and normal for (1 call to PopStatementCG and a source note) so they can be further split.  The patch also s/pn->pn_left/forHead/ and s/pn->pn_right/forBody/.
Comment 5 Jason Orendorff [:jorendorff] 2011-10-21 09:58:43 PDT
Comment on attachment 568547 [details] [diff] [review]
hoist EmitFor, split into EmitForIn and EmitNormalFor

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

r=me, with a few nits to pick in the pre-existing code.

::: js/src/frontend/BytecodeGenerator.cpp
@@ +5641,5 @@
> +                                        (forHead->pn_kid1->isOp(JSOP_DEFVAR))
> +                                        ? SRC_DECL_VAR
> +                                        : SRC_DECL_LET) < 0) {
> +        return false;
> +    }

Put the '{' on its own line, please.

@@ +5796,5 @@
> +        ptrdiff_t off = pn->pn_pos.end.lineno;
> +        if (CG_CURRENT_LINE(cg) != (uintN) off) {
> +            if (NewSrcNote2(cx, cg, SRC_SETLINE, off) < 0)
> +                return false;
> +            CG_CURRENT_LINE(cg) = (uintN) off;

This just used 'off' because it was a convenient tmp variable. It could be renamed to lineno, and its type might as well be uintN (though you'd still need one cast, to avoid warnings).

@@ +5830,5 @@
> +    if (forHead->pn_kid2) {
> +        ptrdiff_t beq = EmitJump(cx, cg, JSOP_IFNE, top - CG_OFFSET(cg));
> +        if (beq < 0)
> +            return false;
> +    } else {

The temporary beq is unnecessary here. And after removing it, you might want to combine this with the else-block, which is very similar.
Comment 6 Luke Wagner [:luke] 2011-10-21 10:49:34 PDT
(In reply to Jason Orendorff [:jorendorff] from comment #5)
> > +                                        (forHead->pn_kid1->isOp(JSOP_DEFVAR))
> > +                                        ? SRC_DECL_VAR
> > +                                        : SRC_DECL_LET) < 0) {
> > +        return false;
> > +    }
> 
> Put the '{' on its own line, please.

For the style record: we agreed on irc that for multi-line conditions, the { could stay on the same line if there was no lexical run-on from the condition to the body as is the case here.
Comment 8 Ed Morley [:emorley] 2011-10-22 06:07:24 PDT
https://hg.mozilla.org/mozilla-central/rev/2d6f586ee5b0

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