Closed Bug 509093 Opened 13 years ago Closed 12 years ago

clean up joinEdgesToEntry

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
status1.9.2 --- beta1-fixed

People

(Reporter: dvander, Assigned: dvander)

References

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file, 2 obsolete files)

Attached 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.
Attached 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?
Attached 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: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.