From b01dad12ce29f38b29a8f80bf842f5a77a0c07b8 Mon Sep 17 00:00:00 2001
From: Til <zoodb@gmx.de>
Date: Sat, 2 Apr 2022 21:42:43 +0200
Subject: [PATCH] moveable PhTree

---
 CHANGELOG.md                     |   1 +
 phtree/phtree.h                  |  11 ++-
 phtree/phtree_multimap.h         |  16 ++++-
 phtree/phtree_multimap_d_test.cc |  70 +++++++++++++++++++
 phtree/phtree_test.cc            | 111 +++++++++++++++++++++++++++++++
 phtree/v16/for_each.h            |   6 +-
 phtree/v16/for_each_hc.h         |   6 +-
 phtree/v16/iterator_base.h       |  15 ++---
 phtree/v16/iterator_full.h       |   2 +-
 phtree/v16/iterator_hc.h         |   2 +-
 phtree/v16/iterator_knn_hs.h     |   4 +-
 phtree/v16/iterator_simple.h     |   4 +-
 phtree/v16/phtree_v16.h          |  14 ++--
 13 files changed, 235 insertions(+), 27 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 582bad93..0245af7b 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -6,6 +6,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 
 ## [Unreleased]
 ### Changed
+- Make PhTree and PhTreeMultimap moveable (move-assign/copy). [#18](https://github.com/tzaeschke/phtree-cpp/issues/18)
 - Potentially **BREAKING CHANGE** when using `IsNodeValid()` in provided filters:
   Changed `bit_width_t` from `uin16_t` to `uint32_t`. This improves performance of 3D insert/emplace
   on small datasets by up to 15%. To avoid warnings that meant that the API of `FilterAABB` and `FilterSphere` 
diff --git a/phtree/phtree.h b/phtree/phtree.h
index 54dfd2dd..8250e96a 100644
--- a/phtree/phtree.h
+++ b/phtree/phtree.h
@@ -32,7 +32,6 @@ namespace improbable::phtree {
 template <dimension_t DIM, typename T, typename CONVERTER = ConverterNoOp<DIM, scalar_64_t>>
 class PhTree {
     friend PhTreeDebugHelper;
-    using KeyInternal = typename CONVERTER::KeyInternal;
     using QueryBox = typename CONVERTER::QueryBoxExternal;
     using Key = typename CONVERTER::KeyExternal;
     static constexpr dimension_t DimInternal = CONVERTER::DimInternal;
@@ -42,7 +41,15 @@ class PhTree {
         typename std::conditional<(DIM == DimInternal), QueryPoint, QueryIntersect>::type;
 
   public:
-    explicit PhTree(CONVERTER converter = CONVERTER()) : tree_{converter}, converter_{converter} {}
+    template <typename CONVERTER2 = CONVERTER>
+    explicit PhTree(CONVERTER2&& converter = CONVERTER())
+    : tree_{&converter_}, converter_{converter} {}
+
+    PhTree(const PhTree& other) = delete;
+    PhTree& operator=(const PhTree& other) = delete;
+    PhTree(PhTree&& other) noexcept = default;
+    PhTree& operator=(PhTree&& other) noexcept = default;
+    ~PhTree() noexcept = default;
 
     /*
      *  Attempts to build and insert a key and a value into the tree.
diff --git a/phtree/phtree_multimap.h b/phtree/phtree_multimap.h
index 75540f9f..af45bb8b 100644
--- a/phtree/phtree_multimap.h
+++ b/phtree/phtree_multimap.h
@@ -236,7 +236,21 @@ class PhTreeMultiMap {
     using EndType = decltype(std::declval<v16::PhTreeV16<DIM, BUCKET, CONVERTER>>().end());
 
     explicit PhTreeMultiMap(CONVERTER converter = CONVERTER())
-    : tree_{converter}, converter_{converter}, size_{0} {}
+    : tree_{&converter_}, converter_{converter}, size_{0} {}
+
+    PhTreeMultiMap(const PhTreeMultiMap& other) = delete;
+    PhTreeMultiMap& operator=(const PhTreeMultiMap& other) = delete;
+    PhTreeMultiMap(PhTreeMultiMap&& other) noexcept = default;
+    // PhTreeMultiMap& operator=(PhTreeMultiMap&& other) noexcept = default;
+    PhTreeMultiMap& operator=(PhTreeMultiMap&& other) noexcept {
+        tree_ = std::move(other.tree_);
+        converter_ = std::move(other.converter_);
+        //the_end_ = std::move(other.the_end_);  // TODO THis works, but it is pretty dirty!
+        bucket_dummy_end_ = std::move(other.bucket_dummy_end_);
+        size_ = std::move(other.size_);
+        return *this;
+    }
+    ~PhTreeMultiMap() noexcept = default;
 
     /*
      *  Attempts to build and insert a key and a value into the tree.
diff --git a/phtree/phtree_multimap_d_test.cc b/phtree/phtree_multimap_d_test.cc
index d695ec91..c39758fc 100644
--- a/phtree/phtree_multimap_d_test.cc
+++ b/phtree/phtree_multimap_d_test.cc
@@ -1102,3 +1102,73 @@ TEST(PhTreeMMDTest, SmokeTestTreeAPI) {
     treePtr.clear();
     delete idPtr;
 }
+
+template <typename TREE>
+void test_tree(TREE& tree) {
+    PhPointD<3> p{1, 2, 3};
+
+    // test various operations
+    tree.emplace(p, Id{2});
+    Id id3{3};
+    tree.insert(p, id3);
+    ASSERT_EQ(tree.size(), 3);
+    ASSERT_EQ(tree.find(p)->_i, 3);
+
+    auto q_window = tree.begin_query({p, p});
+    ASSERT_EQ(3, q_window->_i);
+    ++q_window;
+    ASSERT_EQ(2, q_window->_i);
+    ++q_window;
+    ASSERT_EQ(1, q_window->_i);
+    ++q_window;
+    ASSERT_EQ(q_window, tree.end());
+
+    auto q_extent = tree.begin();
+    ASSERT_EQ(3, q_extent->_i);
+    ++q_extent;
+    ASSERT_EQ(2, q_extent->_i);
+    ++q_extent;
+    ASSERT_EQ(1, q_extent->_i);
+    ++q_extent;
+    ASSERT_EQ(q_extent, tree.end());
+
+    auto q_knn = tree.begin_knn_query(10, p, DistanceEuclidean<3>());
+    ASSERT_EQ(3, q_knn->_i);
+    ++q_knn;
+    ASSERT_EQ(2, q_knn->_i);
+    ++q_knn;
+    ASSERT_EQ(1, q_knn->_i);
+    ++q_knn;
+    ASSERT_EQ(q_knn, tree.end());
+
+    ASSERT_EQ(1, tree.erase(p, Id{1}));
+    ASSERT_EQ(2, tree.size());
+    ASSERT_EQ(0, tree.erase(p, Id{1}));
+    ASSERT_EQ(2, tree.size());
+    ASSERT_EQ(1, tree.erase(p, Id{2}));
+    ASSERT_EQ(1, tree.erase(p, Id{3}));
+    ASSERT_TRUE(tree.empty());
+}
+
+TEST(PhTreeTest, TestMoveConstruct) {
+    // Test edge case: only one entry in tree
+    PhPointD<3> p{1, 2, 3};
+    PhTreeMultiMapD<3, Id> tree1;
+    tree1.emplace(p, Id{1});
+
+    TestTree<3, Id> tree{std::move(tree1)};
+    test_tree(tree);
+    tree.~PhTreeMultiMap();
+}
+
+TEST(PhTreeTest, TestMoveAssign) {
+    // Test edge case: only one entry in tree
+    PhPointD<3> p{1, 2, 3};
+    PhTreeMultiMapD<3, Id> tree1;
+    tree1.emplace(p, Id{1});
+
+    TestTree<3, Id> tree{};
+    tree = std::move(tree1);
+    test_tree(tree);
+    tree.~PhTreeMultiMap();
+}
\ No newline at end of file
diff --git a/phtree/phtree_test.cc b/phtree/phtree_test.cc
index 126943ba..34fae600 100644
--- a/phtree/phtree_test.cc
+++ b/phtree/phtree_test.cc
@@ -1077,3 +1077,114 @@ TEST(PhTreeTest, SmokeTestPoint1) {
     ASSERT_EQ(0, tree.size());
     ASSERT_TRUE(tree.empty());
 }
+
+template <typename TREE>
+void test_tree(TREE& tree) {
+    PhPoint<3> p{1, 2, 3};
+
+    // test various operations
+    tree.emplace(p, Id{2});  // already exists
+    Id id3{3};
+    tree.insert(p, id3);  // already exists
+    ASSERT_EQ(tree.size(), 1);
+    ASSERT_EQ(tree.find(p).second()._i, 1);
+    ASSERT_EQ(tree[p]._i, 1);
+
+    auto q_window = tree.begin_query({p, p});
+    ASSERT_EQ(1, q_window->_i);
+    ++q_window;
+    ASSERT_EQ(q_window, tree.end());
+
+    auto q_extent = tree.begin();
+    ASSERT_EQ(1, q_extent->_i);
+    ++q_extent;
+    ASSERT_EQ(q_extent, tree.end());
+
+    auto q_knn = tree.begin_knn_query(10, p, DistanceEuclidean<3>());
+    ASSERT_EQ(1, q_knn->_i);
+    ++q_knn;
+    ASSERT_EQ(q_knn, tree.end());
+
+    ASSERT_EQ(1, tree.erase(p));
+    ASSERT_EQ(0, tree.size());
+    ASSERT_EQ(0, tree.erase(p));
+    ASSERT_EQ(0, tree.size());
+    ASSERT_TRUE(tree.empty());
+}
+
+TEST(PhTreeTest, TestMoveConstruct) {
+    // Test edge case: only one entry in tree
+    PhPoint<3> p{1, 2, 3};
+    TestTree<3, Id> tree1;
+    tree1.emplace(p, Id{1});
+
+    TestTree<3, Id> tree{std::move(tree1)};
+    test_tree(tree);
+    tree.~PhTree();
+}
+
+TEST(PhTreeTest, TestMoveAssign) {
+    // Test edge case: only one entry in tree
+    PhPoint<3> p{1, 2, 3};
+    TestTree<3, Id> tree1;
+    tree1.emplace(p, Id{1});
+
+    TestTree<3, Id> tree{};
+    tree = std::move(tree1);
+    test_tree(tree);
+    tree.~PhTree();
+}
+
+size_t count_pre{0};
+size_t count_post{0};
+size_t count_query{0};
+
+template <dimension_t DIM, typename SCALAR = scalar_64_t>
+struct DebugConverterNoOp : public ConverterPointBase<DIM, SCALAR, SCALAR> {
+    using BASE = ConverterPointBase<DIM, SCALAR, SCALAR>;
+    using Point = typename BASE::KeyExternal;
+    using PointInternal = typename BASE::KeyInternal;
+
+    constexpr const PointInternal& pre(const Point& point) const {
+        ++count_pre;
+        ++const_cast<size_t&>(count_pre_local);
+        return point;
+    }
+
+    constexpr const Point& post(const PointInternal& point) const {
+        ++count_post;
+        ++const_cast<size_t&>(count_post_local);
+        return point;
+    }
+
+    constexpr const PhBox<DIM, SCALAR>& pre_query(const PhBox<DIM, SCALAR>& box) const {
+        ++count_query;
+        ++const_cast<size_t&>(count_query_local);
+        return box;
+    }
+
+    size_t count_pre_local{0};
+    size_t count_post_local{0};
+    size_t count_query_local{0};
+};
+
+TEST(PhTreeTest, TestMoveAssignCustomConverter) {
+    // Test edge case: only one entry in tree
+    PhPoint<3> p{1, 2, 3};
+    auto converter = DebugConverterNoOp<3>();
+    auto tree1 = PhTree<3, Id, DebugConverterNoOp<3>>(converter);
+    tree1.emplace(p, Id{1});
+    ASSERT_GE(tree1.converter().count_pre_local, 1);
+    ASSERT_EQ(tree1.converter().count_pre_local, count_pre);
+
+    PhTree<3, Id, DebugConverterNoOp<3>> tree{};
+    tree = std::move(tree1);
+    // Assert that converter got moved (or copied?):
+    ASSERT_GE(tree.converter().count_pre_local, 1);
+    ASSERT_EQ(tree.converter().count_pre_local, count_pre);
+
+    test_tree(tree);
+    ASSERT_GE(tree.converter().count_pre_local, 2);
+    ASSERT_EQ(tree.converter().count_pre_local, count_pre);
+    tree.~PhTree();
+}
\ No newline at end of file
diff --git a/phtree/v16/for_each.h b/phtree/v16/for_each.h
index 2531f70e..29706187 100644
--- a/phtree/v16/for_each.h
+++ b/phtree/v16/for_each.h
@@ -34,7 +34,7 @@ class ForEach {
     using EntryT = Entry<DIM, T, SCALAR>;
 
   public:
-    ForEach(const CONVERT& converter, CALLBACK_FN& callback, FILTER filter)
+    ForEach(const CONVERT* converter, CALLBACK_FN& callback, FILTER filter)
     : converter_{converter}, callback_{callback}, filter_(std::move(filter)) {}
 
     void Traverse(const EntryT& entry) {
@@ -52,13 +52,13 @@ class ForEach {
             } else {
                 T& value = child.GetValue();
                 if (filter_.IsEntryValid(child_key, value)) {
-                    callback_(converter_.post(child_key), value);
+                    callback_(converter_->post(child_key), value);
                 }
             }
         }
     }
 
-    CONVERT converter_;
+    const CONVERT* converter_;
     CALLBACK_FN& callback_;
     FILTER filter_;
 };
diff --git a/phtree/v16/for_each_hc.h b/phtree/v16/for_each_hc.h
index 46556f1e..01d6e89b 100644
--- a/phtree/v16/for_each_hc.h
+++ b/phtree/v16/for_each_hc.h
@@ -44,7 +44,7 @@ class ForEachHC {
     ForEachHC(
         const KeyInternal& range_min,
         const KeyInternal& range_max,
-        const CONVERT& converter,
+        const CONVERT* converter,
         CALLBACK_FN& callback,
         FILTER filter)
     : range_min_{range_min}
@@ -77,7 +77,7 @@ class ForEachHC {
                     T& value = child.GetValue();
                     if (IsInRange(child_key, range_min_, range_max_) &&
                         filter_.IsEntryValid(child_key, value)) {
-                        callback_(converter_.post(child_key), value);
+                        callback_(converter_->post(child_key), value);
                     }
                 }
             }
@@ -168,7 +168,7 @@ class ForEachHC {
 
     const KeyInternal range_min_;
     const KeyInternal range_max_;
-    CONVERT converter_;
+    const CONVERT* converter_;
     CALLBACK_FN& callback_;
     FILTER filter_;
 };
diff --git a/phtree/v16/iterator_base.h b/phtree/v16/iterator_base.h
index 8fcd6eea..aaeb9101 100644
--- a/phtree/v16/iterator_base.h
+++ b/phtree/v16/iterator_base.h
@@ -38,7 +38,7 @@ class IteratorBase {
     friend PhTreeV16<DIM, T, CONVERT>;
 
   public:
-    explicit IteratorBase(const CONVERT& converter)
+    explicit IteratorBase(const CONVERT* converter)
     : current_result_{nullptr}
     , current_node_{}
     , parent_node_{}
@@ -46,7 +46,7 @@ class IteratorBase {
     , converter_{converter}
     , filter_{FILTER()} {}
 
-    explicit IteratorBase(const CONVERT& converter, FILTER filter)
+    explicit IteratorBase(const CONVERT* converter, FILTER filter)
     : current_result_{nullptr}
     , current_node_{}
     , parent_node_{}
@@ -85,7 +85,7 @@ class IteratorBase {
     }
 
     auto first() const {
-        return converter_.post(current_result_->GetKey());
+        return converter_->post(current_result_->GetKey());
     }
 
     T& second() const {
@@ -107,9 +107,8 @@ class IteratorBase {
     }
 
     [[nodiscard]] bool ApplyFilter(const EntryT& entry) const {
-        return entry.IsNode()
-            ? filter_.IsNodeValid(entry.GetKey(), entry.GetNodePostfixLen() + 1)
-            : filter_.IsEntryValid(entry.GetKey(), entry.GetValue());
+        return entry.IsNode() ? filter_.IsNodeValid(entry.GetKey(), entry.GetNodePostfixLen() + 1)
+                              : filter_.IsEntryValid(entry.GetKey(), entry.GetValue());
     }
 
     void SetCurrentResult(const EntryT* current_result) {
@@ -127,7 +126,7 @@ class IteratorBase {
     }
 
     auto post(const KeyInternal& point) {
-        return converter_.post(point);
+        return converter_->post(point);
     }
 
   private:
@@ -147,7 +146,7 @@ class IteratorBase {
     const EntryT* current_node_;
     const EntryT* parent_node_;
     bool is_finished_;
-    const CONVERT& converter_;
+    const CONVERT* converter_;
     FILTER filter_;
 };
 
diff --git a/phtree/v16/iterator_full.h b/phtree/v16/iterator_full.h
index b60be035..6be55af9 100644
--- a/phtree/v16/iterator_full.h
+++ b/phtree/v16/iterator_full.h
@@ -33,7 +33,7 @@ class IteratorFull : public IteratorBase<T, CONVERT, FILTER> {
     using EntryT = typename IteratorBase<T, CONVERT, FILTER>::EntryT;
 
   public:
-    IteratorFull(const EntryT& root, const CONVERT& converter, FILTER filter)
+    IteratorFull(const EntryT& root, const CONVERT* converter, FILTER filter)
     : IteratorBase<T, CONVERT, FILTER>(converter, filter), stack_{}, stack_size_{0} {
         PrepareAndPush(root.GetNode());
         FindNextElement();
diff --git a/phtree/v16/iterator_hc.h b/phtree/v16/iterator_hc.h
index bf6cccda..441b9b71 100644
--- a/phtree/v16/iterator_hc.h
+++ b/phtree/v16/iterator_hc.h
@@ -53,7 +53,7 @@ class IteratorHC : public IteratorBase<T, CONVERT, FILTER> {
         const EntryT& root,
         const KeyInternal& range_min,
         const KeyInternal& range_max,
-        const CONVERT& converter,
+        const CONVERT* converter,
         FILTER filter)
     : IteratorBase<T, CONVERT, FILTER>(converter, filter)
     , stack_size_{0}
diff --git a/phtree/v16/iterator_knn_hs.h b/phtree/v16/iterator_knn_hs.h
index ddf5bfc1..2ed8c844 100644
--- a/phtree/v16/iterator_knn_hs.h
+++ b/phtree/v16/iterator_knn_hs.h
@@ -57,12 +57,12 @@ class IteratorKnnHS : public IteratorBase<T, CONVERT, FILTER> {
         const EntryT& root,
         size_t min_results,
         const KeyInternal& center,
-        const CONVERT& converter,
+        const CONVERT* converter,
         DISTANCE dist,
         FILTER filter)
     : IteratorBase<T, CONVERT, FILTER>(converter, filter)
     , center_{center}
-    , center_post_{converter.post(center)}
+    , center_post_{converter->post(center)}
     , current_distance_{std::numeric_limits<double>::max()}
     , num_found_results_(0)
     , num_requested_results_(min_results)
diff --git a/phtree/v16/iterator_simple.h b/phtree/v16/iterator_simple.h
index 815979a7..703d56d6 100644
--- a/phtree/v16/iterator_simple.h
+++ b/phtree/v16/iterator_simple.h
@@ -29,7 +29,7 @@ class IteratorSimple : public IteratorBase<T, CONVERT> {
     using EntryT = typename IteratorBase<T, CONVERT>::EntryT;
 
   public:
-    explicit IteratorSimple(const CONVERT& converter) : IteratorBase<T, CONVERT>(converter) {
+    explicit IteratorSimple(const CONVERT* converter) : IteratorBase<T, CONVERT>(converter) {
         this->SetFinished();
     }
 
@@ -37,7 +37,7 @@ class IteratorSimple : public IteratorBase<T, CONVERT> {
         const EntryT* current_result,
         const EntryT* current_node,
         const EntryT* parent_node,
-        CONVERT converter)
+        const CONVERT* converter)
     : IteratorBase<T, CONVERT>(converter) {
         if (current_result) {
             this->SetCurrentResult(current_result);
diff --git a/phtree/v16/phtree_v16.h b/phtree/v16/phtree_v16.h
index fb666370..79c32749 100644
--- a/phtree/v16/phtree_v16.h
+++ b/phtree/v16/phtree_v16.h
@@ -68,11 +68,17 @@ class PhTreeV16 {
         std::is_arithmetic<ScalarExternal>::value, "ScalarExternal must be an arithmetic type");
     static_assert(DIM >= 1 && DIM <= 63, "This PH-Tree supports between 1 and 63 dimensions");
 
-    PhTreeV16(CONVERT& converter = ConverterNoOp<DIM, ScalarInternal>())
+    PhTreeV16(CONVERT* converter)
     : num_entries_{0}
     , root_{MAX_BIT_WIDTH<ScalarInternal> - 1}
-    , the_end_{converter}
-    , converter_{converter} {}
+    , converter_{converter}
+    , the_end_{converter} {}
+
+    PhTreeV16(const PhTreeV16& other) = delete;
+    PhTreeV16& operator=(const PhTreeV16& other) = delete;
+    PhTreeV16(PhTreeV16&& other) noexcept = default;
+    PhTreeV16& operator=(PhTreeV16&& other) noexcept = default;
+    ~PhTreeV16() noexcept = default;
 
     /*
      *  Attempts to build and insert a key and a value into the tree.
@@ -395,8 +401,8 @@ class PhTreeV16 {
     // Contract: root_ contains a Node with 0 or more entries (the root node is the only Node
     // that is allowed to have less than two entries.
     EntryT root_;
+    CONVERT* converter_;
     IteratorEnd<T, CONVERT> the_end_;
-    CONVERT converter_;
 };
 
 }  // namespace improbable::phtree::v16
