Closed Bug 923160 Opened 7 years ago Closed 7 years ago

Disallow initializers in for-of statements

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: wingo, Assigned: wingo)

References

Details

Attachments

(1 file, 1 obsolete file)

Assignee: nobody → wingo
Duplicate of this bug: 922066
Comment on attachment 813145 [details] [diff] [review]
Disallow initializers in for-of statements

This patch started as a cleanup, then I found this bug.  It does a few things:

 (1) Adds PNK_FOROF.
 (2) Removes JSITER_FOR_OF.  Instead, the C++ interface is ForOfIterator.
 (3) Removes initalizer support in for-of.
Attachment #813145 - Flags: review?(jorendorff)
You may be interested in: bug 901987, bug 921551 ;-)
Comment on attachment 813145 [details] [diff] [review]
Disallow initializers in for-of statements

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

Great, r=me with these minor comments addressed.

Sorry for the slow review here.

::: js/src/frontend/ParseNode.h
@@ +268,5 @@
> + *                            bit set
> + *                          pn_kid2: PNK_NAME or destructuring expr
> + *                            to left of 'of'; if pn_kid1, then this
> + *                            is a clone of pn_kid1->pn_head
> + *                          pn_kid3: object expr to right of 'of'

Please delete the word "object" from the comment about pn_kid3; the RHS of for-in or for-of doesn't have to be an object.

::: js/src/frontend/Parser.cpp
@@ +3984,5 @@
>      StmtInfoPC letStmt(context); /* used if blockObj != nullptr. */
>      ParseNode *pn2, *pn3;      /* forHead->pn_kid2 and pn_kid3. */
>      bool isForOf;
>      bool isForInOrOf = pn1 && matchInOrOf(&isForOf);
> +    ParseNodeKind headKind;

How about this instead, eagerly:

    ParseNodeKind headKind = PNK_FORHEAD;
    if (pn1) {
        bool isForOf;
        if (matchInOrOf(&isForOf))
            headKind = isForOf ? PNK_FOROF : PNK_FORIN;
    }

@@ +3989,1 @@
>      if (isForInOrOf) {

Then this would be
    if (headKind == PNK_FORIN || headKind == PNK_FOROF)
and so on.

@@ +5937,5 @@
>          if (!matchInOrOf(&isForOf)) {
>              report(ParseError, false, null(), JSMSG_IN_AFTER_FOR_NAME);
>              return null();
>          }
> +        ParseNodeKind headKind = PNK_FORIN;

Same here, I think.

::: js/src/jsiter.cpp
@@ +1231,5 @@
> +
> +    if (iterable.isObject())
> +        iterableObj = &iterable.toObject();
> +    else
> +        iterableObj = js_ValueToNonNullObject(cx, iterable);

RootedObject iterableObj(cx, ToObject(iterable));
Attachment #813145 - Flags: review?(jorendorff) → review+
Attachment #813145 - Attachment is obsolete: true
Comment on attachment 817177 [details] [diff] [review]
Disallow initializers in for-of statements v2

r=jorendorff, thanks for the review!
Attachment #817177 - Attachment description: Disallow initializers in for-of statements → Disallow initializers in for-of statements v2
Attachment #817177 - Flags: review+
Keywords: checkin-needed
Duplicate of this bug: 921551
Had to s/NULL/nullptr in a few spots to get it to apply cleanly.

https://hg.mozilla.org/integration/mozilla-inbound/rev/639936b37901
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/639936b37901
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
You need to log in before you can comment on or make changes to this bug.