[webkit-reviews] review denied: [Bug 130772] [WTF] Add the move constructor, move assignment operator for HashTable : [Attachment 227851] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Zan Dobersek <zandobersek at gmail.com>'s
request for review:
Bug 130772: [WTF] Add the move constructor, move assignment operator for
HashTable
https://bugs.webkit.org/show_bug.cgi?id=130772

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=227851&action=review


> Source/WTF/wtf/HashTable.h:1190
> +	   , m_iterators(std::move(other.m_iterators))

We need to invalidate other.m_iterators, not move them. The iterators point
back to the table, and it’s not safe to just point the new table at them but
leave them pointing to the old table.

Should initialize m_iterators to nullptr and call other.invalidateIterators()
in the body of the constructor.

> Source/WTF/wtf/HashTable.h:1201
> +	   std::swap(m_table, other.m_table);
> +	   std::swap(m_tableSize, other.m_tableSize);
> +	   std::swap(m_tableSizeMask, other.m_tableSizeMask);
> +	   std::swap(m_keyCount, other.m_keyCount);
> +	   std::swap(m_deletedCount, other.m_deletedCount);

I don’t think we need to write it like this. We should just copy the data
members in the initialization section, and then zero them out here:

    other.m_table = nullptr;

Writing it with std::swap is unnecessarily oblique and doesn’t have any
significant benefits that I can see.

> Source/WTF/wtf/HashTable.h:1221
> +    template<typename Key, typename Value, typename Extractor, typename
HashFunctions, typename Traits, typename KeyTraits>
> +    inline auto HashTable<Key, Value, Extractor, HashFunctions, Traits,
KeyTraits>::operator=(HashTable&& other) -> HashTable&
> +    {
> +	   std::swap(m_table, other.m_table);
> +	   std::swap(m_tableSize, other.m_tableSize);
> +	   std::swap(m_tableSizeMask, other.m_tableSizeMask);
> +	   std::swap(m_keyCount, other.m_keyCount);
> +	   std::swap(m_deletedCount, other.m_deletedCount);
> +
> +#if CHECK_HASHTABLE_ITERATORS
> +	   std::swap(m_iterators, other.m_iterators);
> +#endif
> +#if DUMP_HASHTABLE_STATS_PER_TABLE
> +	   std::swap(m_stats, other.m_stats);
> +#endif
> +
> +	   return *this;
> +    }

This is incorrect. It swaps, and doesn't move.

If we wanted to build move out of swap we’d need to clear out “this” first. But
I see no reason to build move out of swap.

Also, it swaps incorrectly. If we wanted to swap correctly we could use the
existing swap function above that does it correctly.