Closed
Bug 1209351
Opened 8 years ago
Closed 8 years ago
Add to nsTHashTable the ability to remove an already-found entry
Categories
(Core :: XPCOM, defect)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
mozilla44
Tracking | Status | |
---|---|---|
firefox44 | --- | fixed |
People
(Reporter: n.nethercote, Assigned: n.nethercote)
References
Details
Attachments
(6 files)
1.61 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
5.31 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
971 bytes,
patch
|
jrmuizel
:
review+
|
Details | Diff | Splinter Review |
1.10 KB,
patch
|
michal
:
review+
|
Details | Diff | Splinter Review |
2.96 KB,
patch
|
keeler
:
review+
|
Details | Diff | Splinter Review |
3.55 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
This is a follow-up to bug 1202526, which did the same thing for PLDHashTable. Here are two common nsTHashTable usage patterns: > Entry* entry = table.GetEntry(key); > if (!entry) { > return NS_ERROR_BLAH; > } > table.RemoveEntry(key); > Entry* entry = table.GetEntry(key); > if (foo(entry)) { > table.RemoveEntry(key); > } In both cases RemoveEntry() does an unnecessary re-lookup. You could instead use RawRemoveEntry(), but it does not subsequently shrink the table if its capacity gets too low. This bug is about adding a new function that takes an already found entry (like RawRemoveEntry()) but also shrinks the table afterwards if appropriate (like RemoveEntry()).
![]() |
Assignee | |
Comment 1•8 years ago
|
||
Attachment #8667026 -
Flags: review?(nfroyd)
![]() |
Assignee | |
Comment 2•8 years ago
|
||
Attachment #8667027 -
Flags: review?(bzbarsky)
![]() |
Assignee | |
Comment 3•8 years ago
|
||
Attachment #8667028 -
Flags: review?(jmuizelaar)
![]() |
Assignee | |
Comment 4•8 years ago
|
||
Attachment #8667038 -
Flags: review?(mcmanus)
![]() |
Assignee | |
Comment 5•8 years ago
|
||
Attachment #8667040 -
Flags: review?(dkeeler)
![]() |
Assignee | |
Comment 6•8 years ago
|
||
Attachment #8667041 -
Flags: review?(nfroyd)
Updated•8 years ago
|
Attachment #8667028 -
Flags: review?(jmuizelaar) → review+
Updated•8 years ago
|
Attachment #8667038 -
Flags: review?(mcmanus) → review?(michal.novotny)
Attachment #8667040 -
Flags: review?(dkeeler) → review+
![]() |
||
Updated•8 years ago
|
Attachment #8667026 -
Flags: review?(nfroyd) → review+
![]() |
||
Updated•8 years ago
|
Attachment #8667041 -
Flags: review?(nfroyd) → review+
![]() |
||
Comment 7•8 years ago
|
||
When reviewing the toolkit/ patch, I realized one (small) problem is that people might do something like: Entry* e = mTable.GetEntry(x); if (e) { // ... mTable.RemoveEntry(e); ... // do something with e, which points to something completely different } It looks like e will have been destroyed, which may or may not leave it in a state that is sensible, but mTable could also have been resized, which means e may also point to a valid entry...just not the entry we thought it did. WDYT about making RemoveEntry taking an |Entry*&|, so we can null out the pointer from the hashtable code? (Maybe as a followup bug?) That way, people mistakenly using Entry* after removing it would get a loud error about it (a null pointer dereference), rather than things happening to work and then not... I guess RemoveEntry should also have documentation stating that you're not supposed to use the entry after it has been removed, too.
Flags: needinfo?(n.nethercote)
![]() |
Assignee | |
Comment 8•8 years ago
|
||
> WDYT about making RemoveEntry taking an |Entry*&|, so we can null out the
> pointer from the hashtable code?
Not a bad idea...
Flags: needinfo?(n.nethercote)
![]() |
||
Comment 9•8 years ago
|
||
Comment on attachment 8667027 [details] [diff] [review] (part 2) - Optimize nsTHashTable::RemoveEntry() usage in dom/ r=me
Attachment #8667027 -
Flags: review?(bzbarsky) → review+
Updated•8 years ago
|
Attachment #8667038 -
Flags: review?(michal.novotny) → review+
Comment 10•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/114a04484e3b https://hg.mozilla.org/integration/mozilla-inbound/rev/e645ae59fcec https://hg.mozilla.org/integration/mozilla-inbound/rev/469b3a7acb0d https://hg.mozilla.org/integration/mozilla-inbound/rev/43f1095ea9cd https://hg.mozilla.org/integration/mozilla-inbound/rev/e33b7964a131 https://hg.mozilla.org/integration/mozilla-inbound/rev/6dff11f5236e
Comment 11•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/114a04484e3b https://hg.mozilla.org/mozilla-central/rev/e645ae59fcec https://hg.mozilla.org/mozilla-central/rev/469b3a7acb0d https://hg.mozilla.org/mozilla-central/rev/43f1095ea9cd https://hg.mozilla.org/mozilla-central/rev/e33b7964a131 https://hg.mozilla.org/mozilla-central/rev/6dff11f5236e
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in
before you can comment on or make changes to this bug.
Description
•