Closed Bug 1057912 Opened 6 years ago Closed 6 years ago
Privatize most of PL
_DHash Table's fields
PL_DHashTable is a completely public struct. It shouldn't be.
|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.
|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)
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+
> 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.
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.
> 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.
Try looks good: https://tbpl.mozilla.org/?tree=Try&rev=05efb099fb21
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.