Remove extra RHS checking in CheckDestructuring

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: jorendorff, Assigned: jorendorff)

Tracking

Other Branch
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [fixed-in-tracemonkey])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 years ago
In jsparse.cpp, CheckDestructuring raises an error if the destructuring shorthand {p1, p2} appears to the right of =.

That check is insufficient, since there are all sorts of other places where it might erroneously occur-- basically anywhere an expression is used as an rvalue. So js_EmitTree also checks, in the TOK_RC case.

The latter check seems to cover all the cases. CheckDestructuring doesn't need to check, and the code is shorter without it.
(Assignee)

Comment 1

6 years ago
Created attachment 523289 [details] [diff] [review]
v1

Also updated to idiomatic C++.

-25 lines net.

(More where this came from, if I can ever get my def-use chains straight.)
Assignee: general → jorendorff
Attachment #523289 - Flags: review?(brendan)
(Assignee)

Comment 2

6 years ago
Created attachment 523296 [details] [diff] [review]
v2

v1 left the declaration of fpvd even though it was unused. Deleting that leads to more simplifications, so it's a total of -40 lines.
Attachment #523289 - Attachment is obsolete: true
Attachment #523289 - Flags: review?(brendan)
Attachment #523296 - Flags: review?(brendan)
Comment on attachment 523296 [details] [diff] [review]
v2

Nice.

FYI, there is a Harmony proposal to support {p1, p2} for constructing an object with properties of those names, whose initial values come from the result of evaluating those lexical refs.

/be
Attachment #523296 - Flags: review?(brendan) → review+
(Assignee)

Comment 4

6 years ago
http://hg.mozilla.org/tracemonkey/rev/87fc73e2264f
Whiteboard: [fixed-in-tracemonkey]
http://hg.mozilla.org/mozilla-central/rev/87fc73e2264f
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.