Closed Bug 655089 Opened 14 years ago Closed 14 years ago

nsXPConnect::ToParticipant calls js_GetGCThingTraceKind twice

Categories

(Core :: XPConnect, defect)

Other Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla7

People

(Reporter: mccr8, Assigned: mccr8)

Details

Attachments

(1 file, 1 obsolete file)

It looks to me that in nsXPConnect.cpp, nsXPConnect::ToParticipant calls js_GetGCThingTraceKind twice due to macro'age: 420 #define ADD_TO_CC(_kind) ((_kind) == JSTRACE_OBJECT || (_kind) == JSTRACE_XML) [...] 533 nsCycleCollectionParticipant * 534 nsXPConnect::ToParticipant(void *p) 535 { 536 if (!ADD_TO_CC(js_GetGCThingTraceKind(p))) 537 return NULL; 538 return this; 539 } Not a huge deal as this function has no side effects, but it work that doesn't need to be done. This can be fixed by creating a new variable and assigning the result of the function to it, or by turning ADD_TO_CC into an inline function called addToCCKind or something. The latter would be a bit more civilized, if we can rely on the inlining. I used the former fix in my patch for Bug 650519.
Attached patch turn CC_KIND into a call (obsolete) — Splinter Review
I ran this on try, and it looks okay, except mobile looks kind of bad. One of the Maemo builds is burning, which I will retry, and the Android tests are still running, 8 or so hours later.
fixed bitrot
Assignee: nobody → continuation
Attachment #535223 - Attachment is obsolete: true
Comment on attachment 538561 [details] [diff] [review] change CC_KIND macro to a function to avoid double call Passed a try run.
Attachment #538561 - Flags: review?(mrbkap)
Comment on attachment 538561 [details] [diff] [review] change CC_KIND macro to a function to avoid double call Good catch.
Attachment #538561 - Flags: review?(mrbkap) → review+
Keywords: checkin-needed
Attachment #538561 - Flags: checkin?
Comment on attachment 538561 [details] [diff] [review] change CC_KIND macro to a function to avoid double call Landed on inbound.
Attachment #538561 - Flags: checkin? → checkin+
Keywords: checkin-needed
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla7
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: