clean up joinEdgesToEntry

RESOLVED FIXED

Status

()

RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: dvander, Assigned: dvander)

Tracking

unspecified
x86
Mac OS X
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)

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.
Blocks: 459301
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.
Assignee: general → dvander
Attachment #393241 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #393935 - Flags: review?(sayrer)

Updated

9 years ago
Attachment #393935 - Flags: review?(gal) → review+

Comment 3

9 years ago
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 4

9 years ago
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

Comment 7

9 years ago
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

Updated

9 years ago
Attachment #394968 - Flags: review?(gal) → review+

Comment 10

9 years ago
http://hg.mozilla.org/mozilla-central/rev/7a1032f3aac0
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED

Comment 11

9 years ago
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/e0d686f8310b
status1.9.2: --- → beta1-fixed
Flags: wanted1.9.2+
Depends on: 531513
You need to log in before you can comment on or make changes to this bug.