[webkit-reviews] review denied: [Bug 136934] Add a move operator and constructor override to HashSet : [Attachment 238347] patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Roger Fong <roger_fong at apple.com>'s
request for review:
Bug 136934: Add a move operator and constructor override to HashSet
https://bugs.webkit.org/show_bug.cgi?id=136934

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

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