Closed Bug 1123151 Opened 10 years ago Closed 10 years ago

Privatize PLDHashTable::ops

Categories

(Core :: XPCOM, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla38

People

(Reporter: n.nethercote, Assigned: n.nethercote)

References

Details

Attachments

(3 files)

PLDHashTable::ops is public, and frequently used to indicate if a PLDHashTable has been initialized. It can be made private and we can make PLDHashTable nicer to use in the process.
Currently the setting of PLDHashTable::ops is very haphazard. - PLDHashTable has no constructor, so it's not auto-nulled, so lots of places null it themselves. - In the fallible PLDHashTable::Init() function, if the entry storage allocation fails we'll be left with a table that has |ops| set -- indicating it's been initialized -- but has null entry storage. I'm not certain this can cause problems but it feels unsafe, and some (but not all) callers of Init() null it on failure. - PLDHashTable does not null |ops| in Finish(), so some (but not all) callers do this themselves. This patch makes things simpler. - It adds a constructor that zeroes |ops|. - It modifies Init() so that it only sets |ops| once success is ensured. - It zeroes |ops| in Finish(). - Finally, it removes all the now-unnecessary |ops| nulling done by the users of PLDHashTable.
Attachment #8551067 - Flags: review?(nfroyd)
This encapsulates most of the uses of PLDHashTable::ops.
Attachment #8551068 - Flags: review?(nfroyd)
This required adding a getter and a setter, but they're used sparingly.
Attachment #8551069 - Flags: review?(nfroyd)
Comment on attachment 8551067 [details] [diff] [review] (part 1) - Set PLDHashTable::ops consistently Review of attachment 8551067 [details] [diff] [review]: ----------------------------------------------------------------- It'd be good to add checks in xpcom/tests/TestPLDHash.cpp for: - The constructor zero-ing |ops|. - |ops| not being set if |Init()| returns failure. - |Finish()| zero-ing |ops| appropriately. ::: layout/style/nsCSSRuleProcessor.cpp @@ +3341,5 @@ > mCacheKey(aKey), > mSheetType(aSheetType) > { > + unused << PL_DHashTableInit(&mRulesByWeight, &gRulesByWeightOps, > + sizeof(RuleByWeightEntry), fallible_t(), 32); This is an odd construction; I think it's worth a comment as to why we're doing this. Alternatively, you could make this infallible. I don't have the experience to say what a good comment is here, or the repercussions of making this infallible. Ask heycam for review on those? ::: xpcom/glue/pldhash.h @@ +223,5 @@ > > public: > + // All the other fields are initialized in Init(), but we zero |ops| here > + // because it's used to determine if Init() has been called. > + PLDHashTable() : ops(nullptr) {} MOZ_CONSTEXPR this constructor, please.
Attachment #8551067 - Flags: review?(nfroyd) → review+
Attachment #8551068 - Flags: review?(nfroyd) → review+
Comment on attachment 8551069 [details] [diff] [review] (part 3) - Make PLDHashTable::ops private Review of attachment 8551069 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/plugins/base/nsJSNPRuntime.cpp @@ +1959,5 @@ > if (entry->mNpp == nppcx->npp) { > // Prevent invalidate() and deallocate() from touching the hash > // we're enumerating. > + const PLDHashTableOps *ops = table->Ops(); > + table->SetOps(nullptr); This is...gross.
Attachment #8551069 - Flags: review?(nfroyd) → review+
> It'd be good to add checks in xpcom/tests/TestPLDHash.cpp Good idea. > > + unused << PL_DHashTableInit(&mRulesByWeight, &gRulesByWeightOps, > > + sizeof(RuleByWeightEntry), fallible_t(), 32); > > This is an odd construction; I think it's worth a comment as to why we're > doing this. Alternatively, you could make this infallible. > > I don't have the experience to say what a good comment is here, or the > repercussions of making this infallible. Ask heycam for review on those? heycam said making it infallible is fine. The number of entries is small, and each entry is only a few words. > > + // All the other fields are initialized in Init(), but we zero |ops| here > > + // because it's used to determine if Init() has been called. > > + PLDHashTable() : ops(nullptr) {} > > MOZ_CONSTEXPR this constructor, please. I ended up having to zero every member (constexpr constructor require this) but that's ok. > This is...gross. Indeed. I won't pretend to understand if it's necessary; I preserved the functionality and otherwise left it alone.
FWIW, these are all common patterns: > if (t.IsInitialized()) { PL_DHashTableFinish(t); } > > n += t.IsInitialized() ? t.SizeOf{In,Ex}cludingThis(t, ...) : 0; > > if (!t.IsInitialized()) { PL_DHashTableInit(t, ...); } > > if (t.IsInitialized()) { PL_DHashTableEnumerate(t, ...); } I considered changing the PLDHashTable functions so that you could just call them and they would do nothing if that was appropriate (or return zero, in the *SizeOf* case). It would get rid of many IsInitialized() calls. But in the end I didn't, because the other PLDHashTable functions (like Add()) don't really have a sensible no-op, and because this would introduce an inconsistency with js::HashTable.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: