Don't optimize group assignment given holey RHS

VERIFIED FIXED in mozilla1.9.2a1

Status

()

defect
P4
normal
VERIFIED FIXED
11 years ago
10 years ago

People

(Reporter: brendan, Assigned: brendan)

Tracking

({verified1.9.1})

Trunk
mozilla1.9.2a1
Points:
---
Bug Flags:
wanted1.9.1 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment, 3 obsolete attachments)

covered by js1_8/regress/regress-469625-02.js
Flags: in-testsuite+
Posted patch fix (obsolete) — Splinter Review
Assignee: general → brendan
Status: NEW → ASSIGNED
Attachment #369342 - Flags: review?(mrbkap)
Priority: -- → P4
Attachment #369342 - Flags: review?(mrbkap) → review+
(In reply to comment #2)
> Created an attachment (id=369342) [details]
> fix

This does not remove the code in EmitGroupAssignment that deals with holes in the right-hand-side. Any reason for keeping this deadwood?
(In reply to comment #3)
> (In reply to comment #2)
> > Created an attachment (id=369342) [details] [details]
> > fix
> 
> This does not remove the code in EmitGroupAssignment that deals with holes in
> the right-hand-side. Any reason for keeping this deadwood?

Nope, I was just reading that, while killing time at the TC39 meeting, then forgot to remove when I patched.

/be
Posted patch fix, v2 (obsolete) — Splinter Review
Attachment #369342 - Attachment is obsolete: true
Attachment #369411 - Flags: review?(igor)
There is another pre-existing deadwood in EmitGroupAssignment. MaybeEmitGroupAssignment calls it only when lhs->pn_count <= rhs->pn_count. But the second loop over lhs does not use this condition and makes sure that extra undefs are placed on the stack to initialize elements from the left without the corresponding part on the right like in [a,b] = [1]. So either the condition MaybeEmitGroupAssignment or the push code in the EmitGroupAssignment loop can be removed.
Attachment #369411 - Flags: review?(igor) → review+
Comment on attachment 369411 [details] [diff] [review]
fix, v2

Note that the patch would need a merge with changes due to the bug 484769.
Posted patch fix, v3 (obsolete) — Splinter Review
One more time, just to make sure. Jet-lag is hurting me...

/be
Attachment #369411 - Attachment is obsolete: true
Attachment #369922 - Flags: review?(igor)
Attachment #369922 - Flags: review?(igor) → review+
Attachment #369922 - Attachment is obsolete: true
Attachment #376370 - Flags: review+
Fixed:

http://hg.mozilla.org/tracemonkey/rev/a4197febbf1d
http://hg.mozilla.org/mozilla-central/rev/834e62999a36

/be
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: fixed-in-tracemonkey
Target Milestone: --- → mozilla1.9.2a1
Looks like this burned the static analysis boxes
Ah, bsmedberg says on irc:

sayrer: I'm bisecting the static analysis burnage... there was a treehydra commit around the same time which might have exposed a latent older SM issue
Yeah, I looked last night, but it wasn't me! ;-)

/be
Flags: wanted1.9.1+
v 1.9.1, 1.9.2
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.