ConcurrentHashMap: Make iterators not copyable

Summary: Disable unsafe iterator operations: copying and operator++ (because it copies the iterator). Copying is unsafe because starting to protect buckets and nodes after their removal is not guaranteed to be effective.

Reviewed By: yfeldblum, djwatson

Differential Revision: D8594743

fbshipit-source-id: 0990d8fe979a3217db855641d23e24cc853a25bf

folly/concurrency/ConcurrentHashMap.h

@@ -405,17 +405,11 @@ class ConcurrentHashMap {
     }
 
     ConstIterator& operator++() {
-      it_++;
+      ++it_;
       next();
       return *this;
     }
 
-    ConstIterator operator++(int) {
-      auto prev = *this;
-      ++*this;
-      return prev;
-    }
-
     bool operator==(const ConstIterator& o) const {
       return it_ == o.it_ && segment_ == o.segment_;
     }
@@ -424,12 +418,7 @@ class ConcurrentHashMap {
       return !(*this == o);
     }
 
-    ConstIterator& operator=(const ConstIterator& o) {
-      parent_ = o.parent_;
-      it_ = o.it_;
-      segment_ = o.segment_;
-      return *this;
-    }
+    ConstIterator& operator=(const ConstIterator& o) = delete;
 
     ConstIterator& operator=(ConstIterator&& o) noexcept {
       if (this != &o) {
@@ -440,11 +429,7 @@ class ConcurrentHashMap {
       return *this;
     }
 
-    ConstIterator(const ConstIterator& o) {
-      parent_ = o.parent_;
-      it_ = o.it_;
-      segment_ = o.segment_;
-    }
+    ConstIterator(const ConstIterator& o) = delete;
 
     ConstIterator(ConstIterator&& o) noexcept
         : it_(std::move(o.it_)),

folly/concurrency/detail/ConcurrentHashMap-detail.h

@@ -728,12 +728,6 @@ class alignas(64) ConcurrentHashMapSegment {
       }
     }
 
-    Iterator operator++(int) {
-      auto prev = *this;
-      ++*this;
-      return prev;
-    }
-
     bool operator==(const Iterator& o) const {
       return node_ == o.node_;
     }
@@ -742,24 +736,20 @@ class alignas(64) ConcurrentHashMapSegment {
       return !(*this == o);
     }
 
-    Iterator& operator=(const Iterator& o) {
-      node_ = o.node_;
-      hazptrs_[1].reset(node_);
-      idx_ = o.idx_;
-      buckets_ = o.buckets_;
-      hazptrs_[0].reset(buckets_);
-      bucket_count_ = o.bucket_count_;
+    Iterator& operator=(const Iterator& o) = delete;
+
+    Iterator& operator=(Iterator&& o) noexcept {
+      if (this != &o) {
+        hazptrs_ = std::move(o.hazptrs_);
+        node_ = std::exchange(o.node_, nullptr);
+        buckets_ = std::exchange(o.buckets_, nullptr);
+        bucket_count_ = std::exchange(o.bucket_count_, 0);
+        idx_ = std::exchange(o.idx_, 0);
+      }
       return *this;
     }
 
-    /* implicit */ Iterator(const Iterator& o) {
-      node_ = o.node_;
-      hazptrs_[1].reset(node_);
-      idx_ = o.idx_;
-      buckets_ = o.buckets_;
-      hazptrs_[0].reset(buckets_);
-      bucket_count_ = o.bucket_count_;
-    }
+    Iterator(const Iterator& o) = delete;
 
     Iterator(Iterator&& o) noexcept
         : hazptrs_(std::move(o.hazptrs_)),

folly/concurrency/test/ConcurrentHashMapTest.cpp

@@ -233,15 +233,15 @@ TEST(ConcurrentHashMap, MapIterateTest) {
   EXPECT_NE(iter, foomap.cend());
   EXPECT_EQ(iter->first, 1);
   EXPECT_EQ(iter->second, 1);
-  iter++;
+  ++iter;
   EXPECT_NE(iter, foomap.cend());
   EXPECT_EQ(iter->first, 2);
   EXPECT_EQ(iter->second, 2);
-  iter++;
+  ++iter;
   EXPECT_EQ(iter, foomap.cend());
 
   int count = 0;
-  for (auto it = foomap.cbegin(); it != foomap.cend(); it++) {
+  for (auto it = foomap.cbegin(); it != foomap.cend(); ++it) {
     count++;
   }
   EXPECT_EQ(count, 2);
@@ -271,8 +271,7 @@ TEST(ConcurrentHashMap, EraseTest) {
 TEST(ConcurrentHashMap, CopyIterator) {
   ConcurrentHashMap<int, int> map;
   map.insert(0, 0);
-  auto const cbegin = map.cbegin();
-  for (auto cit = cbegin; cit != map.cend(); ++cit) {
+  for (auto cit = map.cbegin(); cit != map.cend(); ++cit) {
     std::pair<int const, int> const ckv{0, 0};
     EXPECT_EQ(*cit, ckv);
   }
@@ -459,7 +458,7 @@ TEST(ConcurrentHashMap, IterateStressTest) {
           EXPECT_TRUE(res);
         }
         int count = 0;
-        for (auto it = m.cbegin(); it != m.cend(); it++) {
+        for (auto it = m.cbegin(); it != m.cend(); ++it) {
           printf("Item is %li\n", it->first);
           if (it->first < 10) {
             count++;