Const map/set iteration.

Summary: In the current state, it was not possible to iterate, or even access a const map, or const set structure because of an incorrect implementation of "ConstAccessors".

Reviewers: mislav.bradac, teon.banek, buda

Reviewed By: teon.banek, buda

Subscribers: pullbot

Differential Revision: https://phabricator.memgraph.io/D902

src/data_structures/concurrent/common.hpp

@@ -42,9 +42,11 @@ class Item : public TotalOrdering<Item<K, T>>,
 
 // Common base for accessor of all derived containers(ConcurrentMap,
 // ConcurrentSet, ...) from SkipList.
-template <typename T>
+template <typename T, bool IsConst>
 class AccessorBase {
-  typedef SkipList<T> list;
+  typedef
+      typename std::conditional<IsConst, const SkipList<T>, SkipList<T>>::type
+          list;
   typedef typename SkipList<T>::Iterator list_it;
   typedef typename SkipList<T>::ConstIterator list_it_con;
 
@@ -60,20 +62,20 @@ class AccessorBase {
 
   size_t size() { return accessor.size(); };
 
-  list_it begin() { return accessor.begin(); }
+  auto begin() { return accessor.begin(); }
 
-  list_it_con begin() const { return accessor.cbegin(); }
+  auto begin() const { return accessor.cbegin(); }
 
   list_it_con cbegin() const { return accessor.cbegin(); }
 
-  list_it end() { return accessor.end(); }
+  auto end() { return accessor.end(); }
 
-  list_it_con end() const { return accessor.cend(); }
+  auto end() const { return accessor.cend(); }
 
   list_it_con cend() const { return accessor.cend(); }
 
   size_t size() const { return accessor.size(); }
 
  protected:
-  typename list::template Accessor<SkipList<T>> accessor;
+  decltype(std::declval<list>().access()) accessor;
 };

src/data_structures/concurrent/concurrent_map.hpp

@@ -21,13 +21,14 @@ class ConcurrentMap {
  public:
   ConcurrentMap() {}
 
-  class Accessor : public AccessorBase<item_t> {
+  template <bool IsConst = false>
+  class Accessor : public AccessorBase<item_t, IsConst> {
     friend class ConcurrentMap;
 
-    using AccessorBase<item_t>::AccessorBase;
+    using AccessorBase<item_t, IsConst>::AccessorBase;
 
    private:
-    using AccessorBase<item_t>::accessor;
+    using AccessorBase<item_t, IsConst>::accessor;
 
    public:
     std::pair<list_it, bool> insert(const K &key, const T &data) {
@@ -71,10 +72,8 @@ class ConcurrentMap {
     bool remove(const K &key) { return accessor.remove(key); }
   };
 
-  Accessor access() { return Accessor(&skiplist); }
-
-  // TODO:
-  // const Accessor access() const { return Accessor(&skiplist); }
+  auto access() { return Accessor<false>(&skiplist); }
+  auto access() const { return Accessor<true>(&skiplist); }
 
  private:
   list skiplist;

src/data_structures/concurrent/concurrent_set.hpp

@@ -14,13 +14,14 @@ class ConcurrentSet {
  public:
   ConcurrentSet() {}
 
-  class Accessor : public AccessorBase<T> {
+  template <bool IsConst = false>
+  class Accessor : public AccessorBase<T, IsConst> {
     friend class ConcurrentSet;
 
-    using AccessorBase<T>::AccessorBase;
+    using AccessorBase<T, IsConst>::AccessorBase;
 
    private:
-    using AccessorBase<T>::accessor;
+    using AccessorBase<T, IsConst>::accessor;
 
    public:
     std::pair<list_it, bool> insert(const T &item) {
@@ -60,9 +61,8 @@ class ConcurrentSet {
     bool remove(const T &item) { return accessor.remove(item); }
   };
 
-  Accessor access() { return Accessor(&skiplist); }
-
-  const Accessor access() const { return Accessor(&skiplist); }
+  auto access() { return Accessor<false>(&skiplist); }
+  auto access() const { return Accessor<true>(&skiplist); }
 
  private:
   list skiplist;

src/data_structures/concurrent/skiplist.hpp

@@ -475,37 +475,11 @@ class SkipList : private Lockable<lock_t> {
       status_.alive_ = false;
     }
 
-    // TODO(dgleich): Remove after C++17 compile flag.
-    template <class B>
-    struct negation : std::integral_constant<bool, !bool(B::value)> {};
-
-    template <typename TIsConst = TSkipList>
-    Iterator begin(typename std::enable_if<
-                       negation<std::is_const<TIsConst>>::value>::type * = 0) {
-      return skiplist->begin();
-    }
-
-    template <typename TIsConst = TSkipList>
-    ConstIterator begin(
-        typename std::enable_if<std::is_const<TIsConst>::value>::type * =
-            0) const {
-      return skiplist->cbegin();
-    }
+    auto begin() const { return skiplist->begin(); }
 
     ConstIterator cbegin() const { return skiplist->cbegin(); }
 
-    template <typename TIsConst = TSkipList>
-    Iterator end(typename std::enable_if<
-                     negation<std::is_const<TIsConst>>::value>::type * = 0) {
-      return skiplist->end();
-    }
-
-    template <typename TIsConst = TSkipList>
-    ConstIterator end(
-        typename std::enable_if<std::is_const<TIsConst>::value>::type * =
-            0) const {
-      return skiplist->cend();
-    }
+    auto end() const { return skiplist->end(); }
 
     ConstIterator cend() const { return skiplist->cend(); }
 
@@ -700,7 +674,7 @@ class SkipList : private Lockable<lock_t> {
 
   Accessor<SkipList> access() { return Accessor<SkipList>(this); }
 
-  Accessor<const SkipList> caccess() const {
+  Accessor<const SkipList> access() const {
     return Accessor<const SkipList>(this);
   }
 

src/storage/locking/record_lock.cpp

@@ -18,7 +18,7 @@ namespace {
 // returned.
 std::experimental::optional<tx::transaction_id_t> FindOldestTxInLockCycle(
     tx::transaction_id_t start,
-    ConcurrentMap<tx::transaction_id_t, tx::transaction_id_t>::Accessor
+    ConcurrentMap<tx::transaction_id_t, tx::transaction_id_t>::Accessor<>
         &graph_accessor) {
   std::vector<tx::transaction_id_t> path;
   std::unordered_set<tx::transaction_id_t> visited;

tests/concurrent/common.hpp

@@ -40,7 +40,7 @@ auto rand_gen_bool(size_t n = 1) {
 
 // Checks for all owned keys if there data is data.
 template <typename S>
-void check_present_same(typename S::Accessor &acc, size_t data,
+void check_present_same(typename S::template Accessor<> &acc, size_t data,
                         std::vector<size_t> &owned) {
   for (auto num : owned) {
     CHECK(acc.find(num)->second == data) << "My data is present and my";
@@ -49,7 +49,7 @@ void check_present_same(typename S::Accessor &acc, size_t data,
 
 // Checks for all owned.second keys if there data is owned.first.
 template <typename S>
-void check_present_same(typename S::Accessor &acc,
+void check_present_same(typename S::template Accessor<> &acc,
                         std::pair<size_t, std::vector<size_t>> &owned) {
   check_present_same<S>(acc, owned.first, owned.second);
 }
@@ -74,7 +74,7 @@ void check_size_list(S &acc, long long size) {
       << iterator_counter;
 }
 template <typename S>
-void check_size(typename S::Accessor &acc, long long size) {
+void check_size(typename S::template Accessor<> &acc, long long size) {
   // check size
 
   CHECK(acc.size() == size)
@@ -94,7 +94,7 @@ void check_size(typename S::Accessor &acc, long long size) {
 
 // Checks if order in list is maintened. It expects map
 template <typename S>
-void check_order(typename S::Accessor &acc) {
+void check_order(typename S::template Accessor<> &acc) {
   if (acc.begin() != acc.end()) {
     auto last = acc.begin()->first;
     for (auto elem : acc) {
@@ -122,10 +122,9 @@ void check_set(DynamicBitset<> &db, std::vector<bool> &set) {
 // Runs given function in threads_no threads and returns vector of futures for
 // there
 // results.
-template <class R, typename S>
-std::vector<std::future<std::pair<size_t, R>>> run(
-    size_t threads_no, S &skiplist,
-    std::function<R(typename S::Accessor, size_t)> f) {
+template <class R, typename S, class FunT>
+std::vector<std::future<std::pair<size_t, R>>> run(size_t threads_no,
+                                                   S &skiplist, FunT f) {
   std::vector<std::future<std::pair<size_t, R>>> futures;
 
   for (size_t thread_i = 0; thread_i < threads_no; ++thread_i) {
@@ -181,7 +180,7 @@ std::vector<bool> collect_set(
 // Returns object which tracs in owned which (key,data) where added and
 // downcounts.
 template <class K, class D, class S>
-auto insert_try(typename S::Accessor &acc, long long &downcount,
+auto insert_try(typename S::template Accessor<> &acc, long long &downcount,
                 std::vector<K> &owned) {
   return [&](K key, D data) mutable {
     if (acc.insert(key, data).second) {

tests/concurrent/sl_set.cpp

@@ -16,6 +16,7 @@ int main(int argc, char **argv) {
 
   auto futures =
       run<std::vector<long>>(THREADS_NO, skiplist, [](auto acc, auto index) {
+
         auto rand = rand_gen(key_range);
         auto rand_op = rand_gen_bool(no_insert_for_one_delete);
         long long downcount = op_per_thread;

tests/unit/concurrent_map.cpp

@@ -8,7 +8,7 @@
 
 using concurrent_map_t = ConcurrentMap<int, int>;
 
-void print_skiplist(const concurrent_map_t::Accessor &map) {
+void print_skiplist(const concurrent_map_t::Accessor<false> &map) {
   DLOG(INFO) << "Map now has: ";
   for (auto &kv : map)
     DLOG(INFO) << fmt::format("    ({}, {})", kv.first, kv.second);

tests/unit/concurrent_map_access.cpp

@@ -0,0 +1,47 @@
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+#include <vector>
+
+#include "data_structures/concurrent/concurrent_map.hpp"
+
+TEST(ConcurrentMap, Access) {
+  ConcurrentMap<int, int> input;
+  {
+    auto accessor = input.access();
+    accessor.insert(1, 1);
+    accessor.insert(2, 2);
+    accessor.insert(3, 3);
+  }
+
+  auto accessor = input.access();
+  std::vector<int> results;
+  for (auto it = accessor.begin(); it != accessor.end(); ++it)
+    results.push_back(it->first);
+
+  EXPECT_THAT(results, testing::ElementsAre(1, 2, 3));
+}
+
+TEST(ConcurrentMap, ConstAccess) {
+  ConcurrentMap<int, int> input;
+  {
+    auto accessor = input.access();
+    accessor.insert(1, 1);
+    accessor.insert(2, 2);
+    accessor.insert(3, 3);
+  }
+
+  const ConcurrentMap<int, int> &map = input;
+  auto accessor = map.access();
+
+  std::vector<int> results;
+  for (auto it = accessor.begin(); it != accessor.end(); ++it)
+    results.push_back(it->first);
+
+  EXPECT_THAT(results, testing::ElementsAre(1, 2, 3));
+}
+
+int main(int argc, char **argv) {
+  ::testing::InitGoogleTest(&argc, argv);
+  return RUN_ALL_TESTS();
+}

tests/unit/concurrent_set.cpp

@@ -6,7 +6,7 @@
 #include "data_structures/concurrent/concurrent_set.hpp"
 #include "utils/assert.hpp"
 
-void print_skiplist(const ConcurrentSet<int>::Accessor &skiplist) {
+void print_skiplist(const ConcurrentSet<int>::Accessor<false> &skiplist) {
   DLOG(INFO) << "Skiplist set now has:";
   for (auto &item : skiplist) DLOG(INFO) << item;
 }

tests/unit/skiplist_access.cpp

@@ -32,7 +32,7 @@ TEST(SkipList, ConstAccess) {
   }
 
   const SkipList<int> &skiplist = input;
-  auto accessor = skiplist.caccess();
+  auto accessor = skiplist.access();
 
   std::vector<int> results;
   for (auto it = accessor.begin(); it != accessor.end(); ++it)