Bug 5332 - Make HashMap/HashSet support non-POD types
Summary: Make HashMap/HashSet support non-POD types
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 420+
Hardware: Mac OS X 10.4
: P2 Normal
Assignee: Maciej Stachowiak
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2005-10-10 19:37 PDT by Maciej Stachowiak
Modified: 2005-12-22 18:33 PST (History)
0 users

See Also:


Attachments
blind stab at it (7.23 KB, patch)
2005-10-10 19:39 PDT, Maciej Stachowiak
no flags Details | Formatted Diff | Diff
a real fix, that I actually tested and know works (60.31 KB, patch)
2005-12-21 15:25 PST, Maciej Stachowiak
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Maciej Stachowiak 2005-10-10 19:37:46 PDT
It would be really useful if HashMap and HashSet supported non-POD types, so that they could use 
SharedPtrs for instance as key and value types.
Comment 1 Maciej Stachowiak 2005-10-10 19:39:47 PDT
Created attachment 4303 [details]
blind stab at it

JavaScriptCore and WebCore compile and work, but I did not performance test,
nor did I check that it works as advertised for any non-POD types, SharedPtr or
otherwise. For types besides SharedPtr you have to make your own HashTraits.
Comment 2 Maciej Stachowiak 2005-10-11 01:06:55 PDT
Darin points out I should use swap. But I would need to figure out the right way to do swap generically 
while letting classes specialize it to avoid redundant ref/deref.
Comment 3 Maciej Stachowiak 2005-12-21 15:25:57 PST
Created attachment 5211 [details]
a real fix, that I actually tested and know works

Here's a real fix. I tested it and was careful to avoid gratuitous copying.
Comment 4 Darin Adler 2005-12-21 20:24:03 PST
Comment on attachment 5211 [details]
a real fix, that I actually tested and know works

I see some tabs in the ChangeLog.

There's a sentence fragment in the ChangeLog.

That space you removed from array_object.cpp is one I included intentionally.
When I have to add a space at the end of a template declaration, I like to add
a corresponding space at the start so it looks symmetrical. We could
standardize our handling of this -- I'd be happy to switch to your usage.

This line:

+	 ValueType tmp(key,mapped);

is missing a space after a comma.

In HashMap.h you sometimes say std::pair and other times just pair. You should
consistently use just pair.

It's a bit annoying that you asked someone on our team to fix the "is integer"
thing, and then I "coerced" Geoff into doing it this afternoon, and then you
removed the code that uses it!

I would have left out the swap member function for RefPtr, made the inline swap
function a friend, and had it use std::swap to swap the two m_ptr fields
instead of hand-writing a swap.

r=me
Comment 5 Maciej Stachowiak 2005-12-22 02:37:23 PST
I will fix the minor issues before landing but I think I may keep the swap() member function because it 
seems to be common practice to do it that way. I have locally merged my change with Geoff's isInteger fix 
which is great IMO.