1121760 - Remove PL_DHashTable* functions

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Nicholas Nethercote [inactive]]

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.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: (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).

Comment 2

[Nicholas Nethercote [inactive]]

I'll take this, but I have a whole lot of other PLDHashTable clean-up patches I want to land first.

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Remove PL_DHashTableSearch()

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Remove PL_DHashTableAdd()

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - Remove PL_DHashTableRemove()

Comment 6

[Nicholas Nethercote [inactive]]

Attachment: (part 4) - Remove PL_DHashTableRawRemove()

Comment 7

[Nicholas Nethercote [inactive]]

Attachment: (part 5) - Remove PL_DHashMarkTableImmutable()

Comment 8

[Nicholas Nethercote [inactive]]

Attachment: (part 6) - Move all remaining PL_DHash*() functions into PLDHashTable

Comment 9

[Biru [:poiru]]

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?

Comment 10

[Biru [:poiru]]

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`.

Comment 11

[Nicholas Nethercote [inactive]]

Thank you for the lighting reviews :)

Comment 12

[Nicholas Nethercote [inactive]]

Er, that should be *lightning*...

Comment 13

[Pulsebot]

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

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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