Closed Bug 772292 Opened 13 years ago Closed 13 years ago

Convert JSObject2WrappedJSMap to a new-style HashTable

Categories

(Core :: XPConnect, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla17

People

(Reporter: terrence, Assigned: terrence)

Details

Attachments

(1 file, 1 obsolete file)

Attached patch v0 (obsolete) — Splinter Review
Attachment #640441 - Flags: review?(mrbkap)
Comment on attachment 640441 [details] [diff] [review] v0 Review of attachment 640441 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/xpconnect/src/XPCMaps.cpp @@ +93,5 @@ > + if (JS_IsAboutToBeFinalized(wrapper->GetJSObjectPreserveColor())) > + dying->AppendElement(wrapper); > + } > + } > + wrapper = wrapper->GetNextWrapper(); This line should be inside the while loop. ::: js/xpconnect/src/XPCMaps.h @@ +604,5 @@ > typedef js::HashMap<JSObject *, JSObject *, js::PointerHasher<JSObject *, 3>, > js::SystemAllocPolicy> Map; > > public: > + static JSObject2JSObjectMap* newMap(int size) { Unrelated? @@ +664,5 @@ > } > > private: > JSObject2JSObjectMap() MOZ_DELETE; > + JSObject2JSObjectMap(int size) {} Should remove this constructor, then.
Attached patch v1: With fixes.Splinter Review
(In reply to :Ms2ger from comment #1) > ::: js/xpconnect/src/XPCMaps.cpp > @@ +93,5 @@ > > + if (JS_IsAboutToBeFinalized(wrapper->GetJSObjectPreserveColor())) > > + dying->AppendElement(wrapper); > > + } > > + } > > + wrapper = wrapper->GetNextWrapper(); > > This line should be inside the while loop. Good catch. I made the mistake of trying to understand what this loop was doing instead of just copying it. > ::: js/xpconnect/src/XPCMaps.h > @@ +604,5 @@ > > typedef js::HashMap<JSObject *, JSObject *, js::PointerHasher<JSObject *, 3>, > > js::SystemAllocPolicy> Map; > > > > public: > > + static JSObject2JSObjectMap* newMap(int size) { > > Unrelated? A cleanup of the previous jsdhash I squashed. I figured while I was here I would clean this up a bit more. > @@ +664,5 @@ > > } > > > > private: > > JSObject2JSObjectMap() MOZ_DELETE; > > + JSObject2JSObjectMap(int size) {} > > Should remove this constructor, then. Was the whole point of the above change. Did it for the JSObject2WrappedJS but forgot it here for some reason.
Attachment #640441 - Attachment is obsolete: true
Attachment #640441 - Flags: review?(mrbkap)
Attachment #640653 - Flags: review?(mrbkap)
Previous try run appears to have happened in the middle of complete infrastructure havoc: https://tbpl.mozilla.org/?tree=Try&rev=08c73baedbee
Comment on attachment 640653 [details] [diff] [review] v1: With fixes. Review of attachment 640653 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/xpconnect/src/XPCMaps.h @@ -74,5 @@ > > - ~JSObject2WrappedJSMap(); > -private: > - JSObject2WrappedJSMap(); // no implementation > - JSObject2WrappedJSMap(int size); Should the constructor stay private here to make sure consumers use newMap? @@ -668,5 @@ > } > } > > private: > - JSObject2JSObjectMap() MOZ_DELETE; Ditto here.
Attachment #640653 - Flags: review?(mrbkap) → review+
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: