Closed Bug 646807 Opened 9 years ago Closed 9 years ago

Remove extra RHS checking in CheckDestructuring

Categories

(Core :: JavaScript Engine, defect)

Other Branch
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: jorendorff, Assigned: jorendorff)

Details

(Whiteboard: [fixed-in-tracemonkey])

Attachments

(1 file, 1 obsolete file)

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.
Attached patch v1 (obsolete) — Splinter Review
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)
Attached patch v2Splinter Review
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+
http://hg.mozilla.org/tracemonkey/rev/87fc73e2264f
Whiteboard: [fixed-in-tracemonkey]
http://hg.mozilla.org/mozilla-central/rev/87fc73e2264f
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.