Summary: | Make HashMap/HashSet support non-POD types | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Maciej Stachowiak <mjs> | ||||||
Component: | WebKit Misc. | Assignee: | Maciej Stachowiak <mjs> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | ||||||||
Priority: | P2 | ||||||||
Version: | 420+ | ||||||||
Hardware: | Mac | ||||||||
OS: | OS X 10.4 | ||||||||
Attachments: |
|
Description
Maciej Stachowiak
2005-10-10 19:37:46 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.
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. 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 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
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. |