Closed Bug 700659 Opened 8 years ago Closed 8 years ago

Slay nsHashSets.h

Categories

(Core :: XPCOM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla13

People

(Reporter: khuey, Assigned: froydnj)

References

Details

Attachments

(12 files, 2 obsolete files)

1021 bytes, patch
khuey
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
513 bytes, patch
mak
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
1.01 KB, patch
dbaron
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
933 bytes, patch
bzbarsky
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
4.98 KB, patch
KaiE
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
1.31 KB, patch
bas.schouten
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
2.06 KB, patch
jduell.mcbugs
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
1.28 KB, patch
hsivonen
: review+
smaug
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
1.24 KB, patch
froydnj
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
687 bytes, patch
jrmuizel
: review+
Details | Diff | Splinter Review
4.26 KB, patch
mcmanus
: review+
Details | Diff | Splinter Review
7.94 KB, patch
benjamin
: review+
Details | Diff | Splinter Review
Everything in here is obsolete and nasty and should die.
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.
nsTHashTable<nsFooHashKey> is the replacement.
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.
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.
Attachment #572955 - Flags: review?(benjamin)
Remove offending bits from gfx/thebes/.
Attachment #572956 - Flags: review?(bas.schouten)
Remove offending bits from netwerk/cache/
Attachment #572959 - Flags: review?(jduell.mcbugs)
Attached patch slay nsHashSets in parser/ (obsolete) — Splinter Review
Remove offending bits in parser/html/
Attachment #572960 - Flags: review?(bugs)
Remove offending bits in content/base/.  Oddly enough, this is just dead #includes.
Attachment #572962 - Flags: review?(bzbarsky)
Remove offending bits in security/manager/ssl/.
Attachment #572963 - Flags: review?(kaie)
Attachment #572959 - Attachment description: slay nsHashSets in netwerk/ → slay nsHashSets in layout/
Attachment #572959 - Flags: review?(jduell.mcbugs) → review?(dbaron)
Attachment #572956 - Attachment description: slay nsHashSets in gfx/thebes/ → slay nsHashSets in storage/src/
Attachment #572956 - Flags: review?(bas.schouten) → review?(mak77)
Attachment #572955 - Attachment description: add a Contains method to nsTHashtable → slay nsHashSets in xpcom/base/
Attachment #572955 - Flags: review?(benjamin) → review?(khuey)
Comment on attachment 572960 [details] [diff] [review]
slay nsHashSets in parser/

This patch certainly isn't anything about parser/
Attachment #572960 - Flags: review?(bugs)
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!)
Attachment #572967 - Flags: review?(benjamin)
Remove offending bits in gfx/thebes/
Attachment #572968 - Flags: review?(bas.schouten)
Remove offending bits from netwerk/cache/.
Attachment #572969 - Flags: review?(jduell.mcbugs)
Remove offending bits in parser/html/.
Attachment #572960 - Attachment is obsolete: true
Attachment #572971 - Flags: review?(bugs)
Assignee: nobody → nfroyd
Comment on attachment 572971 [details] [diff] [review]
slay nsHashSets in parser/

I think Henri should look at all the changes happening under parser/
Attachment #572971 - Flags: review?(hsivonen)
Attachment #572971 - Flags: review?(bugs)
Attachment #572971 - Flags: review+
OK, I think everything's in the right place now.  I hate uploading patch series to Bugzilla!
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 on attachment 572962 [details] [diff] [review]
slay nsHashSets in content/

r=me
Attachment #572962 - Flags: review?(bzbarsky) → review+
Attachment #572956 - Flags: review?(mak77) → review+
Attachment #572955 - Flags: review?(khuey) → review+
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.
Attachment #572968 - Flags: review?(bas.schouten) → review+
Attachment #572971 - Flags: review?(hsivonen) → review+
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
Attachment #572967 - Flags: review?(benjamin) → review+
Attachment #572969 - Flags: review?(jduell.mcbugs) → review+
Comment on attachment 572959 [details] [diff] [review]
slay nsHashSets in layout/

r=dbaron
Attachment #572959 - Flags: review?(dbaron) → review+
(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 on attachment 572963 [details] [diff] [review]
slay nsHashSets in security/

r=kaie
Attachment #572963 - Flags: review?(kaie) → review+
I need to fixup nsTHashtable per comment 20.  Then this batch of patches, at least, are ready to go in.
Depends on: 722689
Duplicate of this bug: 405988
Updating to address review comments.  Carrying over r+.
Attachment #572967 - Attachment is obsolete: true
Attachment #599646 - Flags: review+
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!
Status: NEW → ASSIGNED
Keywords: checkin-needed
Whiteboard: [please leave open after merge]
Attachment #572955 - Flags: checkin+
Attachment #572956 - Flags: checkin+
Attachment #572959 - Flags: checkin+
Attachment #572962 - Flags: checkin+
Attachment #572963 - Flags: checkin+
Attachment #572968 - Flags: checkin+
Attachment #572969 - Flags: checkin+
Attachment #572971 - Flags: checkin+
Attachment #599646 - Flags: checkin+
Blocks: 730825
Just an unused #include.
Attachment #600918 - Flags: review?(jmuizelaar)
I think the SPDY work added an nsHashSets usage.
Attachment #600920 - Flags: review?(mcmanus)
Attachment #600922 - Flags: review?(benjamin)
Attachment #600918 - Flags: review?(jmuizelaar) → review+
Attachment #600920 - Flags: review?(mcmanus) → review+
Attachment #600922 - Flags: review?(benjamin) → review+
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!
Keywords: checkin-needed
Whiteboard: [please leave open after merge]
You need to log in before you can comment on or make changes to this bug.