[Webkit-unassigned] [Bug 136934] Add a move operator and constructor override to HashSet

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=136934


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #238347|review?                     |review-
               Flag|                            |




--- Comment #4 from Darin Adler <darin at apple.com>  2014-09-19 08:53:40 PST ---
(From update of attachment 238347)
View in context: https://bugs.webkit.org/attachment.cgi?id=238347&action=review

review- because the EWS compiles failed; I think you are on the right track here, though

> Source/WTF/wtf/HashSet.h:114
> +        HashSet(HashSet&&);
> +        HashSet& operator=(HashSet&&);

We need to add regression tests for this. The file Tools/TestWebKitAPI/Tests/WTF/HashSet.cpp already exists, so we should add tests to it. It should not be hard to test that the set moved from ends up empty and the set moved to ends up with the appropriate contents.

I would have expected that adding these should not have been necessary because HashTable class already has a move constructor and assignment operator, but perhaps there’s something I’m missing. That test will really clear up the mystery.

> Source/WTF/wtf/HashSet.h:315
> +    template<typename Value, typename HashFunctions, typename Traits>
> +    inline HashSet<typename Value, typename HashFunctions, typename Traits>::HashSet(HashSet&& other)
> +    {
> +        swap(other);
> +    }

This should instead be:

    ...
        : m_impl(WTF::move(other.m_impl))
    {
    }

The swap function does unnecessary work. I’m not sure the WTF::move is needed, by the way. Having a regression test is the key.

> Source/WTF/wtf/HashSet.h:323
> +    template<typename T, typename U, typename V>
> +    inline auto HashSet<T, U, V>::operator=(HashSet&& other) -> HashSet&
> +    {
> +        HashSet temp = WTF::move(other);
> +        swap(temp);
> +        return *this;
> +    }

This should instead be:

    ...
    {
        m_impl = WTF::move(other.m_impl);
    }

The swap function does unnecessary work. I’m not sure the WTF::move is needed, by the way. Having a regression test is the key.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.