696108 - extract EmitFor

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Luke Wagner [:luke]]

Attachment: 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]]

Attachment: simplify let expr in for init

Comment 2

[Luke Wagner [:luke]]

Attachment: simplify let expr in for init (with reftest fix)

A reftest caught a change in  the SyntaxError message produced.

Comment 3

[Luke Wagner [:luke]]

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

Attachment: 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]]

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

(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 7

[Luke Wagner [:luke]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2d6f586ee5b0

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/2d6f586ee5b0