Bug 136166 – Make possible HashSet<std::unique_ptr<>>

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Simon Fraser (smfr)]

HashSet<std::unique_ptr<>> should just work, but it does not compile.

[Sam Weinig]

There are two things to solve here:
1) Making it compile (we need to make the HashTraits correct, and add a DefaultHash).
2) Making it nice (e.g. adding built in translation/overloading of raw pointers like we do for RefPtrHashMap).

[Sam Weinig]

Created attachment 238422 [details]
Patch

[Sam Weinig]

(In reply to comment #1)
> There are two things to solve here:
> 1) Making it compile (we need to make the HashTraits correct, and add a DefaultHash).
> 2) Making it nice (e.g. adding built in translation/overloading of raw pointers like we do for RefPtrHashMap).

I decided to do both at once.

[WebKit Commit Bot]

Attachment 238422 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/HashFunctions.h:194:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WTF/wtf/HashSet.h:291:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/HashSet.h:298:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/HashSet.h:305:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/HashSet.h:312:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/OwnPtr.h:78:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/OwnPtr.h:240:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WTF/wtf/HashMap.h:408:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/HashMap.h:415:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/HashMap.h:422:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/HashMap.h:429:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/HashMap.h:439:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/HashMap.h:446:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 13 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sam Weinig]

Created attachment 238426 [details]
Patch

[WebKit Commit Bot]

Attachment 238426 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/HashFunctions.h:194:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WTF/wtf/HashSet.h:291:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/HashSet.h:298:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/HashSet.h:305:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/HashSet.h:312:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/OwnPtr.h:78:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/OwnPtr.h:240:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WTF/wtf/HashMap.h:408:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/HashMap.h:415:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/HashMap.h:422:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/HashMap.h:429:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/HashMap.h:439:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/HashMap.h:446:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 13 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 238426 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=238426&action=review

Wow, looks great!

> Source/WTF/wtf/GetPtr.h:63
> +// Explicit specialization for STL types

I suggest we call these standard library types or C++ standard library types, not STL types. I suggest a period here.

> Source/WTF/wtf/GetPtr.h:65
> +template <typename T> struct IsSmartPtr<std::unique_ptr<T>> {

Should also have Deleter argument here so we can handle any std::unique_ptr, not just one with the standard deleter.

> Source/WTF/wtf/GetPtr.h:70
> +template <typename T>
> +struct GetPtrHelper<std::unique_ptr<T>> {

Should also have Deleter argument here so we can handle any std::unique_ptr, not just one with the standard deleter.

> Source/WTF/wtf/HashFunctions.h:140
> +    template<typename P> struct PtrHash<std::unique_ptr<P>> : PtrHash<P*> {

Should also have Deleter argument here so we can handle any std::unique_ptr, not just one with the standard deleter.

> Source/WTF/wtf/HashFunctions.h:194
> +    template<typename P> struct DefaultHash<std::unique_ptr<P>> { typedef PtrHash<std::unique_ptr<P>> Hash; };

Should also have Deleter argument here so we can handle any std::unique_ptr, not just one with the standard deleter.

> Source/WTF/wtf/HashMap.h:147
> +    // An alternate version of find() for smart pointer keys, that take the raw pointer type as the parameter.

Are you going to follow up by removing RefPtrHashMap and using this technique instead?

Grammatical error here. It should be “takes”, not “take”. Repeated many times in other comments below.

Also, maybe a single comment for all 5 functions rather than a separate paragraph and comment for each?

> Source/WTF/wtf/HashMap.h:406
> +template<typename  K>

Extra space here before K. Same thing four times below.

> Source/WTF/wtf/HashMap.h:444
> +template<typename  K>

Extra space here before K.

> Source/WTF/wtf/HashSet.h:123
> +        template<typename V = ValueType> typename std::enable_if<IsSmartPtr<V>::value, ValueType>::type take(typename GetPtrHelper<V>::PtrType);
> +
> +
>          static bool isValidValue(const ValueType&);

Unwanted extra blank line here.

> Source/WTF/wtf/OwnPtr.h:231
>      }
>  
> +
> +    template<typename P> struct PtrHash<OwnPtr<P>> : PtrHash<P*> {

Unwanted extra blank line here.

> Tools/TestWebKitAPI/Tests/WTF/HashMap.cpp:276
> +    HashMap<std::unique_ptr<ConstructorDestructorCounter>, int> map;

Might consider test coverage for unique_ptr with custom deleter.

[Sam Weinig]

Created attachment 238428 [details]
Patch

[WebKit Commit Bot]

Attachment 238428 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/HashFunctions.h:194:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WTF/wtf/HashSet.h:284:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/HashSet.h:291:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/HashSet.h:298:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/HashSet.h:305:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/OwnPtr.h:78:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/OwnPtr.h:239:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WTF/wtf/HashMap.h:400:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/HashMap.h:407:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/HashMap.h:414:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/HashMap.h:421:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/HashMap.h:431:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/HashMap.h:438:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 13 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sam Weinig]

(In reply to comment #7)
> (From update of attachment 238426 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=238426&action=review
> 
> Wow, looks great!
> 
> > Source/WTF/wtf/GetPtr.h:63
> > +// Explicit specialization for STL types
> 
> I suggest we call these standard library types or C++ standard library types, not STL types. I suggest a period here.

Done and done.

> 
> > Source/WTF/wtf/GetPtr.h:65
> > +template <typename T> struct IsSmartPtr<std::unique_ptr<T>> {
> 
> Should also have Deleter argument here so we can handle any std::unique_ptr, not just one with the standard deleter.

Done (I did it everywhere, not just here).

> 
> > Source/WTF/wtf/HashMap.h:147
> > +    // An alternate version of find() for smart pointer keys, that take the raw pointer type as the parameter.
> 
> Are you going to follow up by removing RefPtrHashMap and using this technique instead?

I'd like to, but I need to first figure out if I have gone far enough.  RefPtrHashMap also has overloads for set() and add(), which I am not sure make sense for std::unique_ptr, though I am not sure they are all that useful for RefPtr anyway (maybe for add() in the case where you don't actually add, you could avoid a ref/deref). If you have thoughts on this, I am all ears.

> 
> Grammatical error here. It should be “takes”, not “take”. Repeated many times in other comments below.
> 
> Also, maybe a single comment for all 5 functions rather than a separate paragraph and comment for each?

Made it one sentence.

> > Source/WTF/wtf/HashMap.h:406
> > +template<typename  K>
> 
> Extra space here before K. Same thing four times below.

Done.

> > Source/WTF/wtf/HashSet.h:123
> > +        template<typename V = ValueType> typename std::enable_if<IsSmartPtr<V>::value, ValueType>::type take(typename GetPtrHelper<V>::PtrType);
> > +
> > +
> >          static bool isValidValue(const ValueType&);
> 
> Unwanted extra blank line here.
> 
> > Source/WTF/wtf/OwnPtr.h:231
> >      }
> >  
> > +
> > +    template<typename P> struct PtrHash<OwnPtr<P>> : PtrHash<P*> {
> 
> Unwanted extra blank line here.

Done.

> 
> > Tools/TestWebKitAPI/Tests/WTF/HashMap.cpp:276
> > +    HashMap<std::unique_ptr<ConstructorDestructorCounter>, int> map;
> 
> Might consider test coverage for unique_ptr with custom deleter.

Done.

[Darin Adler]

(In reply to comment #10)
> RefPtrHashMap also has overloads for set() and add()

I think the point of those is to avoid reference pointer thrash when you pass a PassRefPtr in and put it into the map. We could easily write a test for that to see we can use move semantics to solve the problem without explicit use of PassRefPtr.

[Darin Adler]

Comment on attachment 238428 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=238428&action=review

> Source/WTF/wtf/GetPtr.h:74
> +    static T* getPtr(const std::unique_ptr<T, Deleter>& p) { return const_cast<T*>(p.get()); }

Why was the const_cast needed?

[Sam Weinig]

Committed r173801: <http://trac.webkit.org/changeset/173801>