On February 24, 2015 10:00:36 PM EST, Amarnath V A
+ concurrent_unordered_map(concurrent_unordered_map &&old_map) BOOST_NOEXCEPT : _hasher(std::move(old_map._hasher)), _key_equal(std::move(old_map._key_eq
I think you missed moving a couple of data members from old_map.
+ { + //Hold the rehash lock + typedef decltype(_rehash_lock) rehash_lock_t; + typedef decltype(old_map._rehash_lock) old_rehash_lock_t; + lock_guard
guard(_rehash_lock, adopt_lock_t());
It doesn't make sense to hold the lock for this during move construction. Since the new object does not yet exist, there can be no code using it, much less contending for it.
+ lock_guard
old_guard(old_map._rehash_lock, adopt_lock_t());
It doesn't make sense to me to acquire the lock for the old map during move construction since the old map is a temporary by definition.
+ _buckets.exchange(old_map._buckets.load(memory_order_consume), memory_order_release); + old_map._buckets.exchange(nullptr, memory_order_acq_rel);
Why would you do atomic exchanges after acquiring locks? Perhaps the locks are for another purpose, and atomicity is needed apart from the locks generally, but in the move constructor, that isn't needed.
+ old_map._buckets = new buckets_type(13);
Why would you allocate new buckets for the old map? You should just be ensuring that old_map is destructible or assignable, and I imagine you can do that without allocating new memory.
+ old_map._oldbucketit = old_map._oldbuckets.begin(); + old_map._oldbuckets.fill(nullptr);
Is that needed?
+ } + + concurrent_unordered_map &operator=(concurrent_unordered_map &&old_map) BOOST_NOEXCEPT + { + if (this != &old_map) {
Self move assignment would be pathological. Why not forego this and make it a precondition?
+ //Hold the rehash lock + typedef decltype(_rehash_lock) rehash_lock_t; + typedef decltype(old_map._rehash_lock) old_rehash_lock_t; + lock_guard
guard(_rehash_lock, adopt_lock_t());
Can acquiring the lock throw? That would violate your noexcept declaration.
+ lock_guard
old_guard(old_map._rehash_lock, adopt_lock_t());
The old map is a temporary, so there's no need to acquire its lock.
+ _buckets.exchange(old_map._buckets.load(memory_order_consume), memory_order_release);
Is atomicity really needed here?
+ _hasher = std::move(old_map._hasher); + _key_equal = std::move(old_map._key_equal); + _max_load_factor = std::move(old_map._max_load_factor); + _min_bucket_capacity = std::move(old_map._min_bucket_capacity); + old_map._buckets.exchange(nullptr, memory_order_acq_rel);
Atomicity?
+ old_map._buckets = new buckets_type(13);
Why allocate? Just ensure old_map is destructible or assignable. Also note that new can throw which would violate your noexcept declaration.
+ old_map._oldbucketit = old_map._oldbuckets.begin(); + old_map._oldbuckets.fill(nullptr);
Is that needed? ___ Rob (Sent from my portable computation engine)