From f62695c474e13f0b851e70494c79bb705b0cc4c1 Mon Sep 17 00:00:00 2001
From: Bob Luppes <bobluppes@gmail.com>
Date: Sun, 7 Jan 2024 16:45:13 +0100
Subject: [PATCH] fix: Unexpected move when adding vertices and edges (#186)

* add tests
* implement fix
---
 include/graaflib/graph.tpp   |  8 ++---
 test/graaflib/graph_test.cpp | 70 ++++++++++++++++++++++++++++++++++++
 2 files changed, 74 insertions(+), 4 deletions(-)

diff --git a/include/graaflib/graph.tpp b/include/graaflib/graph.tpp
index ce65b00d..6e3ae12c 100644
--- a/include/graaflib/graph.tpp
+++ b/include/graaflib/graph.tpp
@@ -133,7 +133,7 @@ vertex_id_t graph<VERTEX_T, EDGE_T, GRAPH_TYPE_V>::add_vertex(auto&& vertex) {
     ++vertex_id_supplier_;
   }
   const auto vertex_id{vertex_id_supplier_};
-  vertices_.emplace(vertex_id, std::forward<VERTEX_T>(vertex));
+  vertices_.emplace(vertex_id, std::forward<decltype(vertex)>(vertex));
   return vertex_id;
 }
 
@@ -145,7 +145,7 @@ vertex_id_t graph<VERTEX_T, EDGE_T, GRAPH_TYPE_V>::add_vertex(auto&& vertex,
                                 std::to_string(id) + "]"};
   }
 
-  vertices_.emplace(id, std::forward<VERTEX_T>(vertex));
+  vertices_.emplace(id, std::forward<decltype(vertex)>(vertex));
   return id;
 }
 
@@ -182,13 +182,13 @@ void graph<VERTEX_T, EDGE_T, GRAPH_TYPE_V>::add_edge(vertex_id_t vertex_id_lhs,
   if constexpr (GRAPH_TYPE_V == DIRECTED) {
     adjacency_list_[vertex_id_lhs].insert(vertex_id_rhs);
     edges_.emplace(std::make_pair(vertex_id_lhs, vertex_id_rhs),
-                   std::forward<EDGE_T>(edge));
+                   std::forward<decltype(edge)>(edge));
     return;
   } else if constexpr (GRAPH_TYPE_V == UNDIRECTED) {
     adjacency_list_[vertex_id_lhs].insert(vertex_id_rhs);
     adjacency_list_[vertex_id_rhs].insert(vertex_id_lhs);
     edges_.emplace(detail::make_sorted_pair(vertex_id_lhs, vertex_id_rhs),
-                   std::forward<EDGE_T>(edge));
+                   std::forward<decltype(edge)>(edge));
     return;
   }
 
diff --git a/test/graaflib/graph_test.cpp b/test/graaflib/graph_test.cpp
index 5e21895d..5d6b3b6b 100644
--- a/test/graaflib/graph_test.cpp
+++ b/test/graaflib/graph_test.cpp
@@ -244,4 +244,74 @@ TYPED_TEST(GraphTest, ConstGetter) {
   ASSERT_NO_THROW(test_getters_on_const_graph(graph));
 }
 
+/**
+ * Class which records whether or not it was copy constructed. Can be used to
+ * check whether copy/move semantics are correctly implemented.
+ */
+class copy_recorder {
+ public:
+  // Default ctor
+  copy_recorder() : copied_{false} {}
+
+  // Copy ctor
+  copy_recorder(const copy_recorder &) : copied_{true} {}
+
+  // Move ctor
+  copy_recorder(copy_recorder &&) noexcept : copied_{false} {}
+
+  [[nodiscard]] bool is_copy_constructed() const { return copied_; }
+
+ private:
+  bool copied_{false};
+};
+
+class MoveTest : public testing::Test {
+ protected:
+  using graph_t = directed_graph<copy_recorder, copy_recorder>;
+  graph_t graph_{};
+  copy_recorder moveable_{};
+};
+
+TEST_F(MoveTest, VertexLvalueShouldNotBeMoved) {
+  // WHEN we pass in an l-value
+  const auto id{graph_.add_vertex(moveable_)};
+
+  // THEN the vertex in the graph should be copy constructed
+  EXPECT_TRUE(graph_.get_vertex(id).is_copy_constructed());
+}
+
+TEST_F(MoveTest, VertexRvalueShouldBeMoved) {
+  // WHEN we pass in an r-value
+  const auto id{graph_.add_vertex(std::move(moveable_))};
+
+  // THEN the vertex in the graph should be move constructed
+  EXPECT_FALSE(graph_.get_vertex(id).is_copy_constructed());
+}
+
+TEST_F(MoveTest, EdgeLvalueShouldNotBeMoved) {
+  // GIVEN
+  using vertex_t = graph_t::vertex_t;
+  const auto lhs_vertex{graph_.add_vertex(vertex_t{})};
+  const auto rhs_vertex{graph_.add_vertex(vertex_t{})};
+
+  // WHEN we pass in an l-value
+  graph_.add_edge(lhs_vertex, rhs_vertex, moveable_);
+
+  // THEN the edge in the graph should be copy constructed
+  EXPECT_TRUE(graph_.get_edge(lhs_vertex, rhs_vertex).is_copy_constructed());
+}
+
+TEST_F(MoveTest, EdgeRvalueShouldBeMoved) {
+  // GIVEN
+  using vertex_t = graph_t::vertex_t;
+  const auto lhs_vertex{graph_.add_vertex(vertex_t{})};
+  const auto rhs_vertex{graph_.add_vertex(vertex_t{})};
+
+  // WHEN we pass in an r-value
+  graph_.add_edge(lhs_vertex, rhs_vertex, std::move(moveable_));
+
+  // THEN the edge in the graph should be move constructed
+  EXPECT_FALSE(graph_.get_edge(lhs_vertex, rhs_vertex).is_copy_constructed());
+}
+
 }  // namespace graaf
\ No newline at end of file