Closed
Bug 1175810
Opened 9 years ago
Closed 9 years ago
Remove PL_DHashTableEnumerate() use from dom/
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla42
People
(Reporter: n.nethercote, Assigned: n.nethercote)
References
Details
Attachments
(2 files, 2 obsolete files)
4.31 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
10.30 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
Because PLDHashTable::Iterator is nicer than PL_DHashTableEnumerate().
Assignee | ||
Comment 1•9 years ago
|
||
PLDHashTable::Iterator is so much better. Lots of plumbing removed here.
Attachment #8624042 -
Flags: review?(jonas)
Assignee | ||
Comment 2•9 years ago
|
||
Attachment #8624043 -
Flags: review?(jonas)
I have quite a lot of review backlog these days, and not that much time for reviews, would be great if you could assign this to another dom peer.
Comment 4•9 years ago
|
||
bz, mrbkap and bholley have fairly short review queues, FWIW.
Assignee | ||
Updated•9 years ago
|
Attachment #8624042 -
Flags: review?(jonas) → review?(bzbarsky)
Assignee | ||
Comment 5•9 years ago
|
||
(In reply to Jonas Sicking (:sicking) from comment #3) > I have quite a lot of review backlog these days, and not that much time for > reviews, would be great if you could assign this to another dom peer. FWIW, when I uploaded the patches I looked at review queue lengths and yours was only 2, which was one of the lowest of the DOM peers. Oh well.
Assignee | ||
Updated•9 years ago
|
Attachment #8624043 -
Flags: review?(jonas) → review?(bzbarsky)
Comment 6•9 years ago
|
||
Comment on attachment 8624042 [details] [diff] [review] (part 1) - Remove PL_DHashTableEnumerator() use from nsScriptNameSpaceManager It really doesn't make me happy to give non-const access to the GlobalNameMapEntry to consumers here. Is there a way to do that in the new iterator setup? If not, should we add something to allow that?
Flags: needinfo?(n.nethercote)
Comment 7•9 years ago
|
||
Comment on attachment 8624043 [details] [diff] [review] (part 2) - Remove PL_DHashTableEnumerate() uses from nsJSNPRuntime r=me
Attachment #8624043 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 8•9 years ago
|
||
> It really doesn't make me happy to give non-const access to the
> GlobalNameMapEntry to consumers here. Is there a way to do that in the new
> iterator setup? If not, should we add something to allow that?
Hmm, interesting point. Probably the best thing would be to create a wrapper type for PLDHashTable::Iterator that only gives read-only access. I'll try doing that.
Flags: needinfo?(n.nethercote)
Comment 9•9 years ago
|
||
Comment on attachment 8624042 [details] [diff] [review] (part 1) - Remove PL_DHashTableEnumerator() use from nsScriptNameSpaceManager OK, canceling review for now pending the const goodness. ;)
Attachment #8624042 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 10•9 years ago
|
||
(In reply to Boris Zbarsky [:bz] from comment #6) > Comment on attachment 8624042 [details] [diff] [review] > (part 1) - Remove PL_DHashTableEnumerator() use from nsScriptNameSpaceManager > > It really doesn't make me happy to give non-const access to the > GlobalNameMapEntry to consumers here. Is there a way to do that in the new > iterator setup? If not, should we add something to allow that? bz, would it satisfy you if PLDHashTable::Iterator::Get() returned a |const PLDHashEntryHdr*| instead of a |PLDHashEntryHdr*|? It wouldn't be ironclad because people could always cast around it, but I'm wondering if that would be a good change in general. (PLDHashTable::RemovingIterator::Get() would still return a non-const |PLDHashEntryHdr*|). An iterator wrapper would be able to give stronger guarantees, though.
Flags: needinfo?(bzbarsky)
Comment 11•9 years ago
|
||
> would it satisfy you if PLDHashTable::Iterator::Get() returned a |const PLDHashEntryHdr*|
> instead of a |PLDHashEntryHdr*|?
Yes. I mean, people can cast around the current setup in namespace manager too; it's just an immediate smell if they do.
That said, other cases may want to iterate a hashtable and update all the entries. It's just in this particular namespace manager case that the hashtable is conceptually all readonly.
Flags: needinfo?(bzbarsky)
Assignee | ||
Comment 12•9 years ago
|
||
> That said, other cases may want to iterate a hashtable and update all the > entries. It's just in this particular namespace manager case that the > hashtable is conceptually all readonly. Given the improved PLDHashTable sanity-checking that I'm working on in bug 1131308, I think the const change makes sense -- you're allowed to have multiple Iterators running concurrently, which doesn't make much sense if the entries are being modified. If someone wants non-read-only access they can use PLDHashTable::RemovingIterator(), even if they don't remove any items. (Maybe it should be renamed ModifyingIterator().) Thank you for the quick reviews and replies.
Assignee | ||
Comment 13•9 years ago
|
||
bz, this is much the same as the previous version you looked at, but now this patch applies on top of the patches in bug 1179071 which convert PLDHashTable::Iterator::Get() to return a const pointer.
Attachment #8628060 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•9 years ago
|
Attachment #8624042 -
Attachment is obsolete: true
Comment 14•9 years ago
|
||
Comment on attachment 8628060 [details] [diff] [review] (part 1) - Remove PL_DHashTableEnumerator() use from nsScriptNameSpaceManager r=me, though it might have been nice to have a subclass of PLDHashTable::Iterator here whose Get() returns the right type to start with (by calling PLDHashTable::Iterator::Get() and casting) instead of having consumers cast.
Attachment #8628060 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 15•9 years ago
|
||
> r=me, though it might have been nice to have a subclass of
> PLDHashTable::Iterator here whose Get() returns the right type to start with
> (by calling PLDHashTable::Iterator::Get() and casting) instead of having
> consumers cast.
I considered that here and in some other, similar places. It would have required a bunch of extra boilerplate code just to avoid a couple of inline casts and so I decided against it.
Assignee | ||
Comment 17•9 years ago
|
||
(In reply to Pulsebot from comment #16) > https://hg.mozilla.org/integration/mozilla-inbound/rev/fe1710527c0e Part 2 landed; part 1 is still waiting on bug 1179071.
Keywords: leave-open
Assignee | ||
Comment 19•9 years ago
|
||
bz, apologies for r?ing you a third time. Due to uncertainty in bug 1179071 I just went ahead and made an iterator class that returns const pointers in Get(), like you suggested in comment 14.
Attachment #8629167 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•9 years ago
|
Attachment #8628060 -
Attachment is obsolete: true
Assignee | ||
Comment 20•9 years ago
|
||
The interdiff for the latest version of part 1 works well.
Comment 21•9 years ago
|
||
Comment on attachment 8629167 [details] [diff] [review] (part 1) - Remove PL_DHashTableEnumerator() use from nsScriptNameSpaceManager r=me
Attachment #8629167 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 22•9 years ago
|
||
Thank you for the fast review.
Assignee | ||
Updated•9 years ago
|
Keywords: leave-open
Target Milestone: --- → mozilla42
Assignee | ||
Updated•9 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•