Closed Bug 471703 Opened 12 years ago Closed 12 years ago

Don't optimize group assignment given holey RHS

Categories

(Core :: JavaScript Engine, defect, P4)

defect

Tracking

()

VERIFIED FIXED
mozilla1.9.2a1

People

(Reporter: brendan, Assigned: brendan)

Details

(Keywords: verified1.9.1, Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file, 3 obsolete files)

covered by js1_8/regress/regress-469625-02.js
Flags: in-testsuite+
Attached 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
Attached 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.
Attached 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+
Attached patch fix, v3, rebasedSplinter 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: 12 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.