Closed
Bug 772292
Opened 13 years ago
Closed 13 years ago
Convert JSObject2WrappedJSMap to a new-style HashTable
Categories
(Core :: XPConnect, defect)
Core
XPConnect
Tracking
()
RESOLVED
FIXED
mozilla17
People
(Reporter: terrence, Assigned: terrence)
Details
Attachments
(1 file, 1 obsolete file)
13.50 KB,
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
Try run up at:
https://tbpl.mozilla.org/?tree=Try&rev=ba57767df1f9
Attachment #640441 -
Flags: review?(mrbkap)
Comment 1•13 years ago
|
||
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.
Assignee | ||
Comment 2•13 years ago
|
||
(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)
Assignee | ||
Comment 3•13 years ago
|
||
Previous try run appears to have happened in the middle of complete infrastructure havoc:
https://tbpl.mozilla.org/?tree=Try&rev=08c73baedbee
Comment 4•13 years ago
|
||
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+
Assignee | ||
Comment 5•13 years ago
|
||
Comment 6•13 years ago
|
||
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.
Description
•