Created attachment 393241 [details] [diff] [review] proposed changes 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.
Created attachment 393935 [details] [diff] [review] v2 sayrer noted the control flow in the original patch was still funky. this patch removes the break and continue.
9 years ago
Attachment #393935 - Flags: review?(gal)
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?
Created attachment 394968 [details] [diff] [review] v3 splits some stuff out of the loop, removes redundant setLength()
Attachment #393935 - Attachment is obsolete: true
9 years ago
Attachment #394968 - Flags: review?(gal)
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
status1.9.2: --- → beta1-fixed
You need to log in before you can comment on or make changes to this bug.