Closed
Bug 1057912
Opened 10 years ago
Closed 10 years ago
Privatize most of PL_DHashTable's fields
Categories
(Core :: XPCOM, defect)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
mozilla34
People
(Reporter: n.nethercote, Assigned: n.nethercote)
References
Details
Attachments
(2 files)
14.73 KB,
patch
|
roc
:
review-
|
Details | Diff | Splinter Review |
86.74 KB,
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
PL_DHashTable is a completely public struct. It shouldn't be.
Assignee | ||
Comment 1•10 years ago
|
||
|ops| will need to remain public. Lots of places use this to indicate if a table is live -- both reading and writing it -- so making it private would be pointless because we'd need both a getter and a setter.
Assignee | ||
Comment 2•10 years ago
|
||
|gHashTable| in prefapi.cpp is not static, because it's used also in
nsPrefBranch.cpp and Preferences.cpp. So it's explicitly zeroed at its
definition. But this blocks the privatization of PLDHashTable's fields -- the
explicit zeroing sets the would-be-private fields. And I don't want to add any
constructors to PLDHashTable, because there are lots of static PLDHashTables.
So I've made a wrapper class, |PrefsTable|, that contains just a single static
PLDHashTable, |mTable|, which replaces the old |gHashTable|. That gives me a
hash table that is both static *and* visible in multiple .cpp files.
Attachment #8478749 -
Flags: review?(roc)
Assignee | ||
Comment 3•10 years ago
|
||
This patch privatizes most of the members of of PLDHashTable.
- |ops| is left public, because lots of places null it to indicate if a table
is no longer live.
- |data| is left public, because it's the play-thing of PLDHashTable's users.
- All the PL_DHashTableFoo() functions are left in place, to minimize churn
among PLDHashTable's users. Many of these now immediately call into methods,
e.g. PL_DHashTableFinish() calls PLDHashTable::Finish(). These new methods
are all marked MOZ_ALWAYS_INLINE within pldhash.cpp so the "call onto" step
has no cost, but I could be convinced to do otherwise.
- nsAtomTable.cpp zeroes |entryCount| in one place, at the same time it zeroes
|ops|. I've removed this, and adjusted accordingly the place where
|entryCount| is checked.
- Some declarations had to be moved earlier in pldhash.h.
- I removed PL_DHASH_ENTRY_IS_LIVE, because it was no longer needed.
There are numerous uses of PL_DHASH_FASTCALL in this file. I'm skeptical that
they have any effect, but I've left them in for now.
Attachment #8478750 -
Flags: review?(roc)
Comment on attachment 8478749 [details] [diff] [review]
(part 1) - Make prefapi.cpp's |gHashTable| static
Review of attachment 8478749 [details] [diff] [review]:
-----------------------------------------------------------------
I'm a bit confused by this patch. Why can't you leave the definition of gHashTable as-is and just not give it an explicit initializer?
Attachment #8478749 -
Flags: review?(roc) → review-
Attachment #8478750 -
Flags: review?(roc) → review+
Assignee | ||
Comment 5•10 years ago
|
||
> I'm a bit confused by this patch. Why can't you leave the definition of
> gHashTable as-is and just not give it an explicit initializer?
Because it's not static, it won't be auto-zeroed[*]. And if it's not auto-zeroed, the |ops| checks that are used to determine if it's been instantiated won't be safe.
[*] Actually, I'm now not certain if that is true.
Assignee | ||
Comment 6•10 years ago
|
||
http://stackoverflow.com/questions/1597405/what-happens-to-a-declared-uninitialized-variable-in-c-does-it-have-a-value says that global variables are always zeroed, even if they're not static. So I can just remove the explicitly initialization of |gHashTable| like you suggest.
Assignee | ||
Comment 7•10 years ago
|
||
> These new methods are all marked MOZ_ALWAYS_INLINE within pldhash.cpp so the
> "call onto" step has no cost, but I could be convinced to do otherwise.
A consequence of this is that if you try to use these functions outside of
pldhash.cpp you get a link error. Hmm.
Assignee | ||
Comment 8•10 years ago
|
||
Try looks good:
https://tbpl.mozilla.org/?tree=Try&rev=05efb099fb21
Assignee | ||
Comment 9•10 years ago
|
||
Comment 10•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Assignee | ||
Updated•10 years ago
|
Blocks: modernize-pldhash
You need to log in
before you can comment on or make changes to this bug.
Description
•