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.
Created attachment 238337 [details] patch
Created attachment 238342 [details] patch
Created attachment 238347 [details] patch
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.
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.
(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.
(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.