Closed
Bug 1121202
Opened 9 years ago
Closed 9 years ago
remove PL_DHashTableOperate
Categories
(Core :: XPCOM, defect)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
mozilla38
People
(Reporter: froydnj, Assigned: michael, Mentored)
References
Details
(Whiteboard: [lang=c++])
Attachments
(2 files, 3 obsolete files)
8.03 KB,
patch
|
n.nethercote
:
review+
|
Details | Diff | Splinter Review |
1.92 KB,
patch
|
n.nethercote
:
review+
|
Details | Diff | Splinter Review |
We have these shiny new PL_DHashTable{Add,Remove,Lookup} functions; we don't need the *Operate version anymore.
Reporter | ||
Updated•9 years ago
|
Assignee | ||
Comment 2•9 years ago
|
||
(In reply to Nicholas Nethercote [:njn] from comment #1) > Michael, want to take this one? :) Sure, thanks.
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → michael
Flags: needinfo?(michael)
Assignee | ||
Comment 3•9 years ago
|
||
This patch removes the PL_DHashTableOperate function, which is no longer used. Here are the results from the try server: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b5f6125e91a6
Attachment #8548778 -
Flags: review?(nfroyd)
Comment 4•9 years ago
|
||
jcranmer: this will break comm-central. See bug 1118024 for the kind of conversion that needs to be done. It should be easy, and the resulting patch can be landed right now.
Flags: needinfo?(Pidgeot18)
Reporter | ||
Comment 5•9 years ago
|
||
Comment on attachment 8548778 [details] [diff] [review] Patch Review of attachment 8548778 [details] [diff] [review]: ----------------------------------------------------------------- Thank you for adjusting the comments, too!
Attachment #8548778 -
Flags: review?(nfroyd) → review+
Updated•9 years ago
|
Flags: needinfo?(Pidgeot18)
Comment 6•9 years ago
|
||
This is good, but doesn't go as far as it could. You could also split up PLDHashTable::Operate() and remove PL_DHASH_{ADD,REMOVE,LOOKUP}.
Assignee | ||
Comment 7•9 years ago
|
||
(In reply to Nicholas Nethercote [:njn] from comment #6) > This is good, but doesn't go as far as it could. You could also split up > PLDHashTable::Operate() and remove PL_DHASH_{ADD,REMOVE,LOOKUP}. Agreed. I will make similar changes to the PLDHashTable class in a separate bug.
Reporter | ||
Comment 8•9 years ago
|
||
(In reply to Nicholas Nethercote [:njn] from comment #6) > This is good, but doesn't go as far as it could. You could also split up > PLDHashTable::Operate() and remove PL_DHASH_{ADD,REMOVE,LOOKUP}. I would suggest keeping Operate, as it keeps all the hashing logic in one place; we can rely on the compiler always inlining it. (Moving to a more class-based scheme for this would let us make Operate() private, of course.)
Comment 9•9 years ago
|
||
I think there are also some more references to PL_DHashTableOperate in comments in pldhash.h that your patch doesn't remove. And there's a comment that mentions it in dom/plugins/base/nsJSNPRuntime.cpp.
Assignee | ||
Comment 10•9 years ago
|
||
I've updated the comments to replace all references to PL_DHashTableOperate() with their appropriate replacements.
Attachment #8548778 -
Attachment is obsolete: true
Attachment #8549377 -
Flags: review?(nfroyd)
Attachment #8549377 -
Flags: review?(n.nethercote)
Assignee | ||
Comment 11•9 years ago
|
||
And here's a patch which adds Lookup, Add, and Remove methods to PLDHashTable and makes Operate a private method.
Attachment #8549389 -
Flags: review?(nfroyd)
Attachment #8549389 -
Flags: review?(n.nethercote)
Comment 12•9 years ago
|
||
Comment on attachment 8549377 [details] [diff] [review] Patch Review of attachment 8549377 [details] [diff] [review]: ----------------------------------------------------------------- I don't think this needs two reviews :) Thanks. ::: xpcom/glue/pldhash.h @@ +145,4 @@ > * aside, e.g., to avoid copying live entries into an array of the entry type. > * Copying entry pointers is cheaper, and safe so long as the caller of such a > * "stable" Enumerate doesn't use the set-aside pointers after any call either > + * to PL_DHashTableAdd or PL_DHashTableRemove, or to an "unstable" form of Enumerate, which might Nit: this line is now over 80 chars.
Attachment #8549377 -
Flags: review?(nfroyd)
Attachment #8549377 -
Flags: review?(n.nethercote)
Attachment #8549377 -
Flags: review+
Comment 13•9 years ago
|
||
Comment on attachment 8549389 [details] [diff] [review] Patch Part 2 Review of attachment 8549389 [details] [diff] [review]: ----------------------------------------------------------------- ::: xpcom/glue/pldhash.cpp @@ +664,5 @@ > +PLDHashEntryHdr* > +PLDHashTable::Lookup(const void* aKey) > +{ > + return Operate(aKey, PL_DHASH_LOOKUP); > +} Nit: these should have MOZ_ALWAYS_INLINE on them like Init(), Operate(), etc.
Attachment #8549389 -
Flags: review?(nfroyd)
Attachment #8549389 -
Flags: review?(n.nethercote)
Attachment #8549389 -
Flags: review+
Assignee | ||
Comment 14•9 years ago
|
||
Thanks, Nicholas; I've fixed the formatting issue you noted.
Attachment #8549377 -
Attachment is obsolete: true
Attachment #8549955 -
Flags: review?(n.nethercote)
Assignee | ||
Comment 15•9 years ago
|
||
Thanks, Nicholas; I've marked the Lookup, Add, and Remove methods as MOZ_ALWAYS_INLINE as you suggested.
Attachment #8549389 -
Attachment is obsolete: true
Attachment #8549956 -
Flags: review?(n.nethercote)
Comment 16•9 years ago
|
||
Comment on attachment 8549955 [details] [diff] [review] Patch Review of attachment 8549955 [details] [diff] [review]: ----------------------------------------------------------------- A tip: when somebody gives r+ and has some comments it typically indicates that the changes required are small enough that the reviewer trusts the author to do them without a follow-up review. (If they want a follow-up review they'll typically give r- or f+ and explicitly ask for a follow-up.) But this is not obvious and if in doubt it doesn't hurt to ask again. Thanks. Do you need someone else to land these for you?
Attachment #8549955 -
Flags: review?(n.nethercote) → review+
Updated•9 years ago
|
Attachment #8549956 -
Flags: review?(n.nethercote) → review+
Assignee | ||
Comment 17•9 years ago
|
||
(In reply to Nicholas Nethercote [:njn] from comment #16) > A tip: when somebody gives r+ and has some comments it typically indicates > that the changes required are small enough that the reviewer trusts the > author to do them without a follow-up review. (If they want a follow-up > review they'll typically give r- or f+ and explicitly ask for a follow-up.) > But this is not obvious and if in doubt it doesn't hurt to ask again. Thanks. > > Do you need someone else to land these for you? Thanks for your guidance, Nicholas. I plan to add the checkin-needed keyword once bug 1121482 has been resolved.
Comment 18•9 years ago
|
||
> I plan to add the checkin-needed keyword once bug 1121482 has been resolved. Generally sheriffs are only happy to check-in a patch that has had a try run, so that obviously broken stuff doesn't get landed. (See step 6 in https://developer.mozilla.org/en/docs/Introduction.) Do you have try access? If not, I can do a run for you, but you might like to consider requesting it so you can do it yourself in the future. Here are the relevant docs: https://www.mozilla.org/en-US/about/governance/policies/commit/access-policy/ https://www.mozilla.org/en-US/about/governance/policies/commit/ I'd be happy to vouch for you. Bug 1007046 is an example bug where somebody else got L1 access, which might be helpful.
Flags: needinfo?(michael)
Assignee | ||
Comment 19•9 years ago
|
||
Here are the results from the try server: https://treeherder.mozilla.org/#/jobs?repo=try&revision=12ceb40eb9d3
Flags: needinfo?(michael)
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 20•9 years ago
|
||
Great! Thanks.
Comment 21•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/d4a53fee1e25 https://hg.mozilla.org/integration/mozilla-inbound/rev/d614944d001c
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d4a53fee1e25 https://hg.mozilla.org/mozilla-central/rev/d614944d001c
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Updated•9 years ago
|
Blocks: modernize-pldhash
You need to log in
before you can comment on or make changes to this bug.
Description
•