Status

()

defect
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: dvander, Assigned: dvander)

Tracking

unspecified
x86
macOS
Points:
---
Dependency tree / graph
Bug Flags:
wanted1.9.2 +

Firefox Tracking Flags

(status1.9.2 beta1-fixed)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment, 2 obsolete attachments)

Posted patch proposed changes (obsolete) — Splinter Review
Patch cleans up joinEdgesToEntry.  Preserves functionality except for one detail: it tries to link trees together that have missing globals, but their minimal subset still matches. The old code only matched trees that had completely identical typemaps which is unnecessary.
blocks recursion after all.
Posted patch v2 (obsolete) — Splinter Review
sayrer noted the control flow in the original patch was still funky. this patch removes the break and continue.
Assignee: general → dvander
Attachment #393241 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #393935 - Flags: review?(sayrer)
Attachment #393935 - Flags: review?(gal) → review+
Comment on attachment 393935 [details] [diff] [review]
v2

JS_MIN might call the getters twice. Might want to copy them to variables first.

Maybe split the function into a smaller one for the inner loop? The break is hard to follow. Just a nit. Implement at your leisure.
Comment on attachment 393935 [details] [diff] [review]
v2

yeah, I agree with Andreas, but I can live with it.

OTOH, it looks like 

Queue<unsigned> demotes;

is declared too early, and has setLength called where it needn't be.
Attachment #393935 - Flags: review?(sayrer) → review+
(In reply to comment #4)

I did this to avoid reallocating it on every iteration. If no one cares about that I'll move it down.
Explicit hoisting of an allocation out of a loop could be good -- worth a comment at any rate.

/be
yeah, small thing. I can't really tell if there will be a lot of loops that never need the demotes Queue, and it might not matter at all.

But the setLength() call at the beginning of the while loop can go, right?
Posted patch v3Splinter Review
splits some stuff out of the loop, removes redundant setLength()
Attachment #393935 - Attachment is obsolete: true
Attachment #394968 - Flags: review?(gal) → review+
http://hg.mozilla.org/mozilla-central/rev/7a1032f3aac0
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Depends on: 531513
You need to log in before you can comment on or make changes to this bug.