If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Remove PL_DHashTable* functions

RESOLVED FIXED in Firefox 43

Status

()

Core
XPCOM
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

unspecified
mozilla43
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox43 fixed)

Details

Attachments

(6 attachments, 1 obsolete attachment)

(Assignee)

Description

3 years ago
PLDHashTable now has both C-style functions (e.g. PL_DHashTableInit()) and C++-style methods (e.g. PLDHashTable::Init()). It would be nice to remove the former to avoid code duplication and make the style more like the rest of the codebase.
(Assignee)

Updated

3 years ago
Summary: Remove PL_DHashTable* methods → Remove PL_DHashTable* functions
(Assignee)

Comment 1

3 years ago
Created attachment 8549265 [details] [diff] [review]
(part 1) - Remove PL_DHashTable* functions

I tried removing PL_DHashTableInit() just to see if there were any unexpected
difficulties. There weren't.

Michael, is this something you'd be interested in continuing? If so, it'd
probably make sense to put each function's removal in a separate patch (to
simplify reviewing) but then land them all at once (to simplify updating
comm-central).
(Assignee)

Comment 2

2 years ago
I'll take this, but I have a whole lot of other PLDHashTable clean-up patches I want to land first.
Assignee: nobody → n.nethercote
Blocks: 1128407
(Assignee)

Updated

2 years ago
Attachment #8549265 - Attachment is obsolete: true
(Assignee)

Comment 3

2 years ago
Created attachment 8660767 [details] [diff] [review]
(part 1) - Remove PL_DHashTableSearch()
Attachment #8660767 - Flags: review?(birunthan)
(Assignee)

Comment 4

2 years ago
Created attachment 8660780 [details] [diff] [review]
(part 2) - Remove PL_DHashTableAdd()
Attachment #8660780 - Flags: review?(birunthan)
(Assignee)

Comment 5

2 years ago
Created attachment 8660781 [details] [diff] [review]
(part 3) - Remove PL_DHashTableRemove()
Attachment #8660781 - Flags: review?(birunthan)
(Assignee)

Comment 6

2 years ago
Created attachment 8660786 [details] [diff] [review]
(part 4) - Remove PL_DHashTableRawRemove()
Attachment #8660786 - Flags: review?(birunthan)
(Assignee)

Comment 7

2 years ago
Created attachment 8660787 [details] [diff] [review]
(part 5) - Remove PL_DHashMarkTableImmutable()
Attachment #8660787 - Flags: review?(birunthan)
(Assignee)

Comment 8

2 years ago
Created attachment 8660788 [details] [diff] [review]
(part 6) - Move all remaining PL_DHash*() functions into PLDHashTable
Attachment #8660788 - Flags: review?(birunthan)
Attachment #8660767 - Flags: review?(birunthan) → review+
Comment on attachment 8660780 [details] [diff] [review]
(part 2) - Remove PL_DHashTableAdd()

Review of attachment 8660780 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/cache/nsDiskCacheBinding.cpp
@@ +229,5 @@
>      NS_ASSERTION(initialized, "nsDiskCacheBindery not initialized");
>  
>      // find hash entry for key
> +    auto hashEntry = static_cast<HashTableEntry*>
> +        (table.Add((void *)(uintptr_t) binding->mRecord.HashNumber(), fallible));

Could you remove the space before the * and after the ) while you're here?
Attachment #8660780 - Flags: review?(birunthan) → review+
Attachment #8660781 - Flags: review?(birunthan) → review+
Attachment #8660786 - Flags: review?(birunthan) → review+
Attachment #8660787 - Flags: review?(birunthan) → review+
Comment on attachment 8660788 [details] [diff] [review]
(part 6) - Move all remaining PL_DHash*() functions into PLDHashTable

Review of attachment 8660788 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/glue/pldhash.cpp
@@ +119,5 @@
>    nullptr
>  };
>  
> +/* static */ const PLDHashTableOps*
> +PLDHashTable::StubOps(void)

Please remove `void`.
Attachment #8660788 - Flags: review?(birunthan) → review+
(Assignee)

Comment 11

2 years ago
Thank you for the lighting reviews :)
(Assignee)

Comment 12

2 years ago
Er, that should be *lightning*...

Comment 13

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/e37d0c2d053a
https://hg.mozilla.org/integration/mozilla-inbound/rev/16597175b6f3
https://hg.mozilla.org/integration/mozilla-inbound/rev/108f4bf0585b
https://hg.mozilla.org/integration/mozilla-inbound/rev/f957524fc624
https://hg.mozilla.org/integration/mozilla-inbound/rev/507728088761
https://hg.mozilla.org/integration/mozilla-inbound/rev/36f11da8d2d9
(Assignee)

Updated

2 years ago
Blocks: 1205183
https://hg.mozilla.org/mozilla-central/rev/e37d0c2d053a
https://hg.mozilla.org/mozilla-central/rev/16597175b6f3
https://hg.mozilla.org/mozilla-central/rev/108f4bf0585b
https://hg.mozilla.org/mozilla-central/rev/f957524fc624
https://hg.mozilla.org/mozilla-central/rev/507728088761
https://hg.mozilla.org/mozilla-central/rev/36f11da8d2d9
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox43: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.