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

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted 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 227862: Patch
https://bugs.webkit.org/attachment.cgi?id=227862&action=review

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


> Source/WTF/wtf/HashTable.h:1198
> +    template<typename Key, typename Value, typename Extractor, typename
HashFunctions, typename Traits, typename KeyTraits>
> +    inline HashTable<Key, Value, Extractor, HashFunctions, Traits,
KeyTraits>::HashTable(HashTable&& other)
> +	   : m_table(nullptr)
> +	   , m_tableSize(0)
> +	   , m_tableSizeMask(0)
> +	   , m_keyCount(0)
> +	   , m_deletedCount(0)
> +#if CHECK_HASHTABLE_ITERATORS
> +	   , m_iterators(nullptr)
> +	   , m_mutex(std::make_unique<std::mutex>())
> +#endif
> +#if DUMP_HASHTABLE_STATS_PER_TABLE
> +	   , m_stats(nullptr)
> +#endif
> +    {
> +	   *this = std::move(other);
> +    }

I don’t think it’s good to first make everything null and 0 and only then
overwrite by moving. I suggest we instead just repeat the code from operator=
below here.

> Source/WTF/wtf/HashTable.h:1210
> +	   m_table = std::move(other.m_table);
> +	   m_tableSize = std::move(other.m_tableSize);
> +	   m_tableSizeMask = std::move(other.m_tableSizeMask);
> +	   m_keyCount = std::move(other.m_keyCount);
> +	   m_deletedCount = std::move(other.m_deletedCount);

These uses of std::move don’t accomplish anything. I wish that std::move worked
with scalars in this fashion, but it doesn’t, and it’s not helpful to write
code as if it did. Please omit the calls to std::move and just use normal
assignment.

> Source/WTF/wtf/HashTable.h:1219
> +	   m_stats = std::move(other.m_stats);

Ditto.