Bug 136934 – Add a move operator and constructor override to HashSet

• Component: WebCore Misc.
• Status: NEW

[Roger Fong]

We do this to ensure that the Hashtable fields are zero'ed out when performing a move operation.
fast\dom\html-collections-named-getter.html fails in debug builds without this do a failing assertion that a hashset is empty.

[Roger Fong]

Created attachment 238337 [details]
patch

[Roger Fong]

Created attachment 238342 [details]
patch

[Roger Fong]

Created attachment 238347 [details]
patch

[Darin Adler]

Comment on attachment 238347 [details]
patch

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.

[Anders Carlsson]

We should also make this windows only as well; MSVC seems buggy in this regard - the move ctor and move assignment operator should be generated automatically.

[Darin Adler]

(In reply to comment #5)
> We should also make this windows only as well; MSVC seems buggy in this regard - the move ctor and move assignment operator should be generated automatically.

So it should be:

#if COMPILER(MSVC)

And be documented as a workaround for a compiler bug.

[Anders Carlsson]

(In reply to comment #6)
> (In reply to comment #5)
> > We should also make this windows only as well; MSVC seems buggy in this regard - the move ctor and move assignment operator should be generated automatically.
> 
> So it should be:
> 
> #if COMPILER(MSVC)
> 
> And be documented as a workaround for a compiler bug.

Looking at http://blogs.msdn.com/b/vcblog/archive/2014/06/11/c-11-14-feature-tables-for-visual-studio-14-ctp1.aspx it's likely that it's been fixed in VS 2014, so I think we should check for VS2013 explicitly.