700659 - Slay nsHashSets.h

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Everything in here is obsolete and nasty and should die.

Comment 1

[Nathan Froyd [:froydnj]]

Is there a obvious (templated?) replacement candidate for the types defined therein?  All the HashSets kinds (save for nsInt32HashSet) are used at least once throughout the codebase.

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

nsTHashTable<nsFooHashKey> is the replacement.

Comment 3

[Nathan Froyd [:froydnj]]

Patches incoming shortly to remove *HashSet throughout the code base save for nsCheapSets.  I think nsCheapSets could stand to abandon its pointer-twiddling bits and become templatized, but that's a discussion for a separate bug.  (FWIW, there's only one use of nsCheapSets in the whole tree, so I think it'd be reasonable to make them just a little bit bigger.)

The patches are mostly mechanical and fairly tiny; they could be made even tiner with a little more twiddling.  Will explain in a bit.

Comment 4

[Nathan Froyd [:froydnj]]

Attachment: slay nsHashSets in xpcom/base/

Later patches are much nicer with a Contains method in nsTHashtable.

Later patches could also become even nicer with Put/Remove methods; I opted not to add them because they seemed like clutter.

Comment 5

[Nathan Froyd [:froydnj]]

Attachment: slay nsHashSets in storage/src/

Remove offending bits from gfx/thebes/.

Comment 6

[Nathan Froyd [:froydnj]]

Attachment: slay nsHashSets in layout/

Remove offending bits from netwerk/cache/

Comment 7

[Nathan Froyd [:froydnj]]

Attachment: slay nsHashSets in parser/

Remove offending bits in parser/html/

Comment 8

[Nathan Froyd [:froydnj]]

Attachment: slay nsHashSets in content/

Remove offending bits in content/base/.  Oddly enough, this is just dead #includes.

Comment 9

[Nathan Froyd [:froydnj]]

Attachment: slay nsHashSets in security/

Remove offending bits in security/manager/ssl/.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 572960 [details] [diff] [review]
slay nsHashSets in parser/

This patch certainly isn't anything about parser/

Comment 11

[Nathan Froyd [:froydnj]]

Attachment: add a Contains method to nsTHashtable

Later patches are much nicer with a Contains method in nsTHashtable.

Later patches could also become even nicer with Put/Remove methods; I opted not to add them because they seemed like clutter.

(Now with the correct attachment!)

Comment 12

[Nathan Froyd [:froydnj]]

Attachment: slay nsHashSets in gfx/thebes/

Remove offending bits in gfx/thebes/

Comment 13

[Nathan Froyd [:froydnj]]

Attachment: slay nsHashSets in netwerk/

Remove offending bits from netwerk/cache/.

Comment 14

[Nathan Froyd [:froydnj]]

Attachment: slay nsHashSets in parser/

Remove offending bits in parser/html/.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 572971 [details] [diff] [review]
slay nsHashSets in parser/

I think Henri should look at all the changes happening under parser/

Comment 16

[Nathan Froyd [:froydnj]]

OK, I think everything's in the right place now.  I hate uploading patch series to Bugzilla!

Comment 17

[David Baron :dbaron:]

I'd really much rather have a nicer set API on top of nsTHashtable.  I wrote one once, but never found an excuse to land it, because both times I used it I ended up scrapping that patch.  But it's here:

https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/dba69109eff4/hash-set

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 572962 [details] [diff] [review]
slay nsHashSets in content/

r=me

Comment 19

[Bas Schouten (:bas.schouten)]

Comment on attachment 572968 [details] [diff] [review]
slay nsHashSets in gfx/thebes/

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

Looks fine to me. Not sure if I'm the very best person to review this but I doubt it matters.

Comment 20

[Benjamin Smedberg]

Comment on attachment 572967 [details] [diff] [review]
add a Contains method to nsTHashtable

I think this would be easier to read as:

return !!GetEntry(aKey);

r=me like that

Comment 21

[David Baron :dbaron:]

Comment on attachment 572959 [details] [diff] [review]
slay nsHashSets in layout/

r=dbaron

Comment 22

[David Baron :dbaron:]

(In reply to David Baron [:dbaron] from comment #17)
> I'd really much rather have a nicer set API on top of nsTHashtable.  I wrote
> one once, but never found an excuse to land it, because both times I used it
> I ended up scrapping that patch.  But it's here:
> 
> https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/
> dba69109eff4/hash-set

I filed this as bug 708901.

Comment 23

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

kaie, review ping?

Comment 24

[Kai Engert (:KaiE:)]

Comment on attachment 572963 [details] [diff] [review]
slay nsHashSets in security/

r=kaie

Comment 25

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Is this ready to go?

Comment 26

[Nathan Froyd [:froydnj]]

I need to fixup nsTHashtable per comment 20.  Then this batch of patches, at least, are ready to go in.

Comment 28

[Nathan Froyd [:froydnj]]

Attachment: part 1 - add a Contains method to nsTHashtable

Updating to address review comments.  Carrying over r+.

Comment 29

[Nathan Froyd [:froydnj]]

Please apply "part 1" first; the remaining patches are independent of each other.

Also, there's more work to be done (nsHashSets* needs to be removed once dependent bugs are fixed), so please leave open after the m-c merge.  Thanks!

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7aaa5f1f685e
https://hg.mozilla.org/integration/mozilla-inbound/rev/f66366413f66
https://hg.mozilla.org/integration/mozilla-inbound/rev/a94b06b56773
https://hg.mozilla.org/integration/mozilla-inbound/rev/6735b4f3a4c4
https://hg.mozilla.org/integration/mozilla-inbound/rev/a41cc70ce9c2
https://hg.mozilla.org/integration/mozilla-inbound/rev/b85b6482eb65
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f98fc956d25
https://hg.mozilla.org/integration/mozilla-inbound/rev/2651ff8e19e9
https://hg.mozilla.org/integration/mozilla-inbound/rev/0c9a34063fb2

Comment 31

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/7aaa5f1f685e
https://hg.mozilla.org/mozilla-central/rev/f66366413f66
https://hg.mozilla.org/mozilla-central/rev/a94b06b56773
https://hg.mozilla.org/mozilla-central/rev/6735b4f3a4c4
https://hg.mozilla.org/mozilla-central/rev/a41cc70ce9c2
https://hg.mozilla.org/mozilla-central/rev/b85b6482eb65
https://hg.mozilla.org/mozilla-central/rev/3f98fc956d25
https://hg.mozilla.org/mozilla-central/rev/2651ff8e19e9
https://hg.mozilla.org/mozilla-central/rev/0c9a34063fb2

Comment 32

[Nathan Froyd [:froydnj]]

Attachment: Slay nsHashSets in gfx/thebes/, round 2

Just an unused #include.

Comment 33

[Nathan Froyd [:froydnj]]

Attachment: Slay nsHashSets in netwerk/protocol/

I think the SPDY work added an nsHashSets usage.

Comment 34

[Nathan Froyd [:froydnj]]

Attachment: last part - delete nsHashSets code

Comment 35

[Nathan Froyd [:froydnj]]

Please checkin the netwerk/ and gfx/ parts, then the actual deletion ("last part") patch, otherwise we'll wind up with a broken tree along the way.  Thanks!

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e0913432b56e
https://hg.mozilla.org/integration/mozilla-inbound/rev/949612a18317
https://hg.mozilla.org/integration/mozilla-inbound/rev/bca0ee06670c

Comment 37

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/e0913432b56e
https://hg.mozilla.org/mozilla-central/rev/949612a18317
https://hg.mozilla.org/mozilla-central/rev/bca0ee06670c