Bug 70565 – HashMap should support OwnPtr for mapped values

• Component: Web Template Framework
• Status: RESOLVED FIXED

[Ryosuke Niwa]

We need some traits hacks to make HashMap work well with OwnPtr.

[Darin Adler]

(In reply to comment #0)
> We need some traits hacks to make HashMap work well with OwnPtr.

I presume for values, not keys.

[Ryosuke Niwa]

(In reply to comment #1)
> (In reply to comment #0)
> > We need some traits hacks to make HashMap work well with OwnPtr.
> 
> I presume for values, not keys.

I think just supporting keys is sufficient in most usages.

By the way, it'll be nice if containers that didn't support OwnPtr either had a comment or spit out a compilation error when OwnPtr is used in key or value.

[Adam Klein]

There's more trouble here than just porting over VectorTraits<OwnPtr> to HashTraits<OwnPtr>.  Besides needing to teach HashTable how to copy an OwnPtr, the interface would also need to be tweaked to do the OwnPtr -> raw ptr or OwnPtr -> PassOwnPtr conversion.  For example, some problematic methods:

MappedType get(const KeyType&) const;
MappedType take(const KeyType&);
pair<iterator, bool> set(const KeyType&, const MappedType&);
pair<iterator, bool> add(const KeyType&, const MappedType&);

All of these present some problems if MappedType were to be OwnPtr<T>, problems that aren't present in Vector<OwnPtr<T> > due to the latter's use of references and pointers in its interface.

[Adam Klein]

The easiest solution that comes to mind would be to write a wrapper around HashMap (OwnPtrHashMap?) that takes care of the lifetime management issues and exposes a HashMap-like interface with a few tweaks (e.g., take() should return a PassOwnPtr, set and add should take PassOwnPtrs).  This would require a lot of copying of interface code, but most of the implementation could still be shared with HashMap.

Darin, how does that strike you?

[Darin Adler]

(In reply to comment #4)
> The easiest solution that comes to mind would be to write a wrapper around HashMap (OwnPtrHashMap?) that takes care of the lifetime management issues and exposes a HashMap-like interface with a few tweaks (e.g., take() should return a PassOwnPtr, set and add should take PassOwnPtrs).

We should see how much like RefPtrHashMap we can make this.

[Darin Adler]

I started working on it. It’s easier than it sounds.

I am almost certain that it’s the *values* that we need OwnPtr for. All those cases where we use deleteAllValues are the things that cause trouble.

> It'll be nice if containers that didn't support OwnPtr either had a comment or spit out a compilation error when OwnPtr is used in key or value.

What’s the current status on this? I thought there was a compilation error if you tried to use an OwnPtr as a key already.

[Darin Adler]

Created attachment 114400 [details]
Snapshot of work in progress

[Eric Seidel (no email)]

webkit-patch upload -m "attachment name"
will allow you to set the attachment title when uploading if that's useful to you. :)

[Darin Adler]

Created attachment 114722 [details]
Newer snapshot of work in progress (to be landed in many separate pieces)

[Darin Adler]

(In reply to comment #8)
> webkit-patch upload -m "attachment name"
> will allow you to set the attachment title when uploading if that's useful to you. :)

Thanks. It is useful (if I remember to use it).

[Darin Adler]

In and working as of r101942.

[Darin Adler]

(In reply to comment #4)
> The easiest solution that comes to mind would be to write a wrapper around HashMap (OwnPtrHashMap?) that takes care of the lifetime management issues and exposes a HashMap-like interface with a few tweaks (e.g., take() should return a PassOwnPtr, set and add should take PassOwnPtrs).  This would require a lot of copying of interface code, but most of the implementation could still be shared with HashMap.

What makes the wrapper solution unpleasant and perhaps impractical is the fact that our HashMap class has an iterator, and you can get directly at values in the map through those iterators. The values inside the map need to be OwnPtr. If we made them be raw pointers, then we’d either non-lifetime-safe access to the pointers through the iterators, or we’d have to do some unsafe type punning to treat the raw pointers as OwnPtr. By making the actual entries in the hash table be OwnPtr, those issues don’t come up.