Model dashtable iterator as close as possible to std iterator

core/dash.h

@@ -90,10 +90,7 @@ class DashTable : public detail::DashTableBase {
 
    public:
     using iterator_category = std::forward_iterator_tag;
-    using value_type = const Value_t;
     using difference_type = std::ptrdiff_t;
-    using reference = value_type&;
-    using pointer = value_type*;
 
     // Copy constructor from iterator to const_iterator.
     template <bool TIsConst = IsConst, bool TIsSingleB,
@@ -133,6 +130,19 @@ class DashTable : public detail::DashTableBase {
       return *this;
     }
 
+    detail::IteratorPair<Key_t, Value_t> operator->() {
+      auto* seg = owner_->segment_[seg_id_];
+      return detail::IteratorPair<Key_t, Value_t>{seg->Key(bucket_id_, slot_id_),
+                                                  seg->Value(bucket_id_, slot_id_)};
+    }
+
+    const detail::IteratorPair<Key_t, Value_t> operator->() const {
+      auto* seg = owner_->segment_[seg_id_];
+      return detail::IteratorPair<Key_t, Value_t>{seg->Key(bucket_id_, slot_id_),
+                                                  seg->Value(bucket_id_, slot_id_)};
+    }
+
+#if 0
     const Key_t& key() const {
       return owner_->segment_[seg_id_]->Key(bucket_id_, slot_id_);
     }
@@ -143,22 +153,22 @@ class DashTable : public detail::DashTableBase {
     Key_t* mutable_key() {
       return &owner_->segment_[seg_id_]->Key(bucket_id_, slot_id_);
     };
-
+#endif
     typename std::conditional<IsConst, const Value_t&, Value_t&>::type value() const {
       return owner_->segment_[seg_id_]->Value(bucket_id_, slot_id_);
     }
 
-    pointer operator->() const {
+    /*pointer operator->() const {
       return std::addressof(value());
     }
 
     reference operator*() const {
       return value();
-    }
+    }*/
 
     // Make it self-contained. Does not need container::end().
-    bool is_valid() const {
+    bool is_done() const {
-      return owner_ != nullptr;
+      return owner_ == nullptr;
     }
 
     friend bool operator==(const Iterator& lhs, const Iterator& rhs) {
@@ -411,11 +421,11 @@ size_t DashTable<_Key, _Value, Policy>::Erase(const Key_t& key) {
 template <typename _Key, typename _Value, typename Policy>
 void DashTable<_Key, _Value, Policy>::Erase(iterator it) {
   auto* target = segment_[it.seg_id_];
-  uint64_t key_hash = DoHash(it.key());
+  uint64_t key_hash = DoHash(it->first);
   SegmentIterator sit{it.bucket_id_, it.slot_id_};
 
-  policy_.DestroyKey(*it.mutable_key());
+  policy_.DestroyKey(it->first);
-  policy_.DestroyValue(it.value());
+  policy_.DestroyValue(it->second);
 
   target->Delete(sit, key_hash);
 }

core/dash_internal.h

@@ -93,7 +93,7 @@ template <unsigned NUM_SLOTS> class SlotBitmap {
   };
 
   Unaligned val_[kLen];
-};
+};  // SlotBitmap
 
 template <unsigned NUM_SLOTS, unsigned NUM_STASH_FPS> class BucketBase {
   // We can not allow more than 4 stash fps because we hold stash positions in single byte
@@ -470,6 +470,22 @@ class DashTableBase {
   size_t size_ = 0;
 };  // DashTableBase
 
+template <typename _Key, typename _Value> class IteratorPair {
+ public:
+  IteratorPair(_Key& k, _Value& v) : first(k), second(v) {}
+
+  IteratorPair* operator->() {
+    return this;
+  }
+
+  const IteratorPair* operator->() const {
+    return this;
+  }
+
+  _Key& first;
+  _Value& second;
+};
+
 /***********************************************************
  * Implementation section.
  */

core/dash_test.cc

@@ -358,10 +358,10 @@ TEST_F(DashTest, Insert) {
   EXPECT_EQ(1, dt_.Erase(0));
   EXPECT_EQ(0, dt_.Erase(0));
   auto it = dt_.begin();
-  ASSERT_TRUE(it.is_valid());
+  ASSERT_FALSE(it.is_done());
-  auto some_val = *it;
+  auto some_val = it->second;
   dt_.Erase(it);
-  ASSERT_FALSE(dt_.Find(some_val).is_valid());
+  ASSERT_TRUE(dt_.Find(some_val).is_done());
 }
 
 TEST_F(DashTest, Traverse) {
@@ -372,9 +372,10 @@ TEST_F(DashTest, Traverse) {
   uint64_t cursor = 0;
   vector<unsigned> nums;
   auto tr_cb = [&](const Dash64::iterator& it) {
-    nums.push_back(it.key());
+    nums.push_back(it->first);
-    VLOG(1) << it.bucket_id() << " " << it.slot_id() << " " << it.key();
+    VLOG(1) << it.bucket_id() << " " << it.slot_id() << " " << it->first;
   };
+
   do {
     cursor = dt_.Traverse(cursor, tr_cb);
   } while (cursor != 0);
@@ -394,11 +395,11 @@ TEST_F(DashTest, Bucket) {
   auto it = dt_.begin();
   auto bucket_it = Dash64::bucket_it(it);
 
-  dt_.TraverseBucket(it, [&](auto i) { s.push_back(i.key()); });
+  dt_.TraverseBucket(it, [&](auto i) { s.push_back(i->first); });
 
   unsigned num_items = 0;
-  while (bucket_it.is_valid()) {
+  while (!bucket_it.is_done()) {
-    ASSERT_TRUE(find(s.begin(), s.end(), bucket_it.key()) != s.end());
+    ASSERT_TRUE(find(s.begin(), s.end(), bucket_it->first) != s.end());
     ++bucket_it;
     ++num_items;
   }
@@ -422,8 +423,8 @@ TEST_F(DashTest, Eviction) {
   Dash64::EvictionCb cb = [this](const Dash64::EvictionCandidates& cand) -> unsigned {
     auto it = cand.iter[0];
     unsigned res = 0;
-    for (; it.is_valid(); ++it) {
+    for (; !it.is_done(); ++it) {
-      LOG(INFO) << "Deleting " << it.key();
+      LOG(INFO) << "Deleting " << it->first;
       dt_.Erase(it);
       ++res;
     }