Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: luke, Assigned: luke)

Tracking

unspecified
mozilla10
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

6 years ago
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; ...; ...)'
Attachment #568408 - Flags: review?(jorendorff)
(Assignee)

Comment 1

6 years ago
Created attachment 568409 [details] [diff] [review]
simplify let expr in for init
Attachment #568409 - Flags: review?(jorendorff)
(Assignee)

Comment 2

6 years ago
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.
Attachment #568409 - Attachment is obsolete: true
Attachment #568409 - Flags: review?(jorendorff)
Attachment #568415 - Flags: review?(jorendorff)
(Assignee)

Comment 3

6 years ago
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 :)
Attachment #568415 - Flags: review?(jorendorff)
(Assignee)

Updated

6 years ago
Summary: simplify 'for (let (x=expr) expr; ...; ...)' and extract EmitFor → extract EmitFor
(Assignee)

Comment 4

6 years ago
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/.
Attachment #568408 - Attachment is obsolete: true
Attachment #568415 - Attachment is obsolete: true
Attachment #568408 - Flags: review?(jorendorff)
Attachment #568547 - Flags: review?(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.
Attachment #568547 - Flags: review?(jorendorff) → review+
(Assignee)

Comment 6

6 years ago
(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.
(Assignee)

Comment 7

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d6f586ee5b0
Target Milestone: --- → mozilla10
https://hg.mozilla.org/mozilla-central/rev/2d6f586ee5b0
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.