[webkit-reviews] review granted: [Bug 136166] Make possible HashSet<std::unique_ptr<>> : [Attachment 238426] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Sam Weinig <sam at webkit.org>'s request
for review:
Bug 136166: Make possible HashSet<std::unique_ptr<>>
https://bugs.webkit.org/show_bug.cgi?id=136166

Attachment 238426: Patch
https://bugs.webkit.org/attachment.cgi?id=238426&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.