From 527d4871c7826d5a07686c5e3f1f6edb18fbdc3d Mon Sep 17 00:00:00 2001 From: vrouvrea Date: Mon, 3 Sep 2018 15:00:41 +0000 Subject: Code review : Fix remarks for move and copy assignment git-svn-id: svn+ssh://scm.gforge.inria.fr/svnroot/gudhi/branches/simplex_tree_fix_vincent@3868 636b058d-ea47-450e-bf9e-a15bfbe3eedb Former-commit-id: 069e920ab3ca9504b5e1c49ab23afe1693f4e6f8 --- src/Simplex_tree/include/gudhi/Simplex_tree.h | 93 ++++++++++++---------- .../test/simplex_tree_ctor_and_move_unit_test.cpp | 22 +++-- 2 files changed, 67 insertions(+), 48 deletions(-) diff --git a/src/Simplex_tree/include/gudhi/Simplex_tree.h b/src/Simplex_tree/include/gudhi/Simplex_tree.h index 69ed5e13..20b527a2 100644 --- a/src/Simplex_tree/include/gudhi/Simplex_tree.h +++ b/src/Simplex_tree/include/gudhi/Simplex_tree.h @@ -296,15 +296,15 @@ class Simplex_tree { dimension_(-1) { } /** \brief User-defined copy constructor reproduces the whole tree structure. */ - Simplex_tree(const Simplex_tree& simplex_source) - : null_vertex_(simplex_source.null_vertex_), - root_(nullptr, null_vertex_, simplex_source.root_.members_), + Simplex_tree(const Simplex_tree& complex_source) + : null_vertex_(complex_source.null_vertex_), + root_(nullptr, null_vertex_, complex_source.root_.members_), filtration_vect_(), - dimension_(simplex_source.dimension_) { + dimension_(complex_source.dimension_) { #ifdef DEBUG_TRACES std::cout << "Simplex_tree copy constructor" << std::endl; #endif // DEBUG_TRACES - auto root_source = simplex_source.root_; + auto root_source = complex_source.root_; rec_copy(&root_, &root_source); } @@ -324,34 +324,32 @@ class Simplex_tree { } /** \brief User-defined move constructor relocates the whole tree structure. - * \exception std::invalid_argument In debug mode, if the simplex_source is invalid. + * \exception std::invalid_argument In debug mode, if the complex_source is invalid. */ - Simplex_tree(Simplex_tree && simplex_source) - : null_vertex_(std::move(simplex_source.null_vertex_)), - root_(std::move(simplex_source.root_)), - filtration_vect_(std::move(simplex_source.filtration_vect_)), - dimension_(std::move(simplex_source.dimension_)) { + Simplex_tree(Simplex_tree && complex_source) + : null_vertex_(std::move(complex_source.null_vertex_)), + root_(std::move(complex_source.root_)), + filtration_vect_(std::move(complex_source.filtration_vect_)), + dimension_(std::move(complex_source.dimension_)) { #ifdef DEBUG_TRACES std::cout << "Simplex_tree move constructor" << std::endl; #endif // DEBUG_TRACES // Need to update root members (children->oncles and children need to point on the new root pointer) for (auto& map_el : root_.members()) { - // children->oncles update after the move - if (map_el.second.children()->oncles() == &(simplex_source.root_)) - // reset with the moved root_ pointer value + if (map_el.second.children()->oncles() == &(complex_source.root_)) { + // reset children->oncles with the moved root_ pointer value map_el.second.children()->oncles_ = &root_; - else + } else { // if simplex is of dimension 0, oncles_ shall be nullptr GUDHI_CHECK(map_el.second.children()->oncles_ == nullptr, std::invalid_argument("Simplex_tree move constructor from an invalid Simplex_tree")); - // children update after the move - if (map_el.second.children() == &(simplex_source.root_)) - // reset with the moved root_ pointer value + // and children points on root_ - to be moved map_el.second.assign_children(&root_); + } } // just need to set dimension_ on source to make it available again // (filtration_vect_ and members are already set from the move) - simplex_source.dimension_ = -1; + complex_source.dimension_ = -1; } /** \brief Destructor; deallocates the whole tree structure. */ @@ -364,17 +362,24 @@ class Simplex_tree { } /** \brief User-defined copy assignment reproduces the whole tree structure. */ - Simplex_tree& operator= (const Simplex_tree& simplex_source) + Simplex_tree& operator= (const Simplex_tree& complex_source) { #ifdef DEBUG_TRACES - std::cout << "copy assignment" << std::endl; + std::cout << "Simplex_tree copy assignment" << std::endl; #endif // DEBUG_TRACES - null_vertex_ = simplex_source.null_vertex_; + null_vertex_ = complex_source.null_vertex_; filtration_vect_.clear(); - dimension_ = simplex_source.dimension_; - auto root_source = simplex_source.root_; - // Here a copy will be done - root_ = Siblings(nullptr, null_vertex_); + dimension_ = complex_source.dimension_; + auto root_source = complex_source.root_; + + // We start by deleting root_ if not empty + for (auto sh = root_.members().begin(); sh != root_.members().end(); ++sh) { + if (has_children(sh)) { + rec_delete(sh->second.children()); + } + } + + // root members copy root_.members() = Dictionary(boost::container::ordered_unique_range, root_source.members().begin(), root_source.members().end()); // Needs to reassign children for (auto& map_el : root_.members()) { @@ -385,35 +390,41 @@ class Simplex_tree { } /** \brief User-defined move assignment relocates the whole tree structure. - * \exception std::invalid_argument In debug mode, if the simplex_source is invalid. + * \exception std::invalid_argument In debug mode, if the complex_source is invalid. */ - Simplex_tree& operator=(Simplex_tree&& simplex_source) + Simplex_tree& operator=(Simplex_tree&& complex_source) { #ifdef DEBUG_TRACES - std::cout << "move assignment" << std::endl; + std::cout << "Simplex_tree move assignment" << std::endl; #endif // DEBUG_TRACES // Self-assignment detection - if (&simplex_source != this) { - std::swap( null_vertex_, simplex_source.null_vertex_ ); - std::swap( root_, simplex_source.root_ ); - std::swap( filtration_vect_, simplex_source.filtration_vect_ ); - std::swap( dimension_, simplex_source.dimension_ ); + if (&complex_source != this) { + std::swap( null_vertex_, complex_source.null_vertex_ ); + std::swap( root_, complex_source.root_ ); + std::swap( filtration_vect_, complex_source.filtration_vect_ ); + std::swap( dimension_, complex_source.dimension_ ); // Need to update root members (children->oncles and children need to point on the new root pointer) for (auto& map_el : root_.members()) { - // children->oncles update after the move - if (map_el.second.children()->oncles() == &(simplex_source.root_)) - // reset with the moved root_ pointer value + if (map_el.second.children()->oncles() == &(complex_source.root_)) { + // reset children->oncles with the moved root_ pointer value map_el.second.children()->oncles_ = &root_; - else + } else { // if simplex is of dimension 0, oncles_ shall be nullptr GUDHI_CHECK(map_el.second.children()->oncles_ == nullptr, std::invalid_argument("Simplex_tree move constructor from an invalid Simplex_tree")); - // children update after the move - if (map_el.second.children() == &(simplex_source.root_)) - // reset with the moved root_ pointer value + // and children points on root_ - to be moved map_el.second.assign_children(&root_); + } + } + // complex_source root_ deletion + for (auto sh = complex_source.root_.members().begin(); sh != complex_source.root_.members().end(); ++sh) { + if (has_children(sh)) { + rec_delete(sh->second.children()); + } } + complex_source.root_.members().clear(); + complex_source.dimension_ = -1; } return *this; } diff --git a/src/Simplex_tree/test/simplex_tree_ctor_and_move_unit_test.cpp b/src/Simplex_tree/test/simplex_tree_ctor_and_move_unit_test.cpp index 95e99afe..6c27a458 100644 --- a/src/Simplex_tree/test/simplex_tree_ctor_and_move_unit_test.cpp +++ b/src/Simplex_tree/test/simplex_tree_ctor_and_move_unit_test.cpp @@ -75,6 +75,8 @@ BOOST_AUTO_TEST_CASE_TEMPLATE(simplex_copy_constructor, Simplex_tree, list_of_te std::cout << "********************************************************************" << std::endl; std::cout << "TEST OF COPY ASSIGNMENT" << std::endl; Simplex_tree st3; + // To check there is no memory leak + st3.insert_simplex_and_subfaces({9, 10, 11}, 200.0); st3 = st; print_simplex_filtration(st3, "First copy assignment from the default Simplex_tree"); Simplex_tree st4; @@ -108,12 +110,18 @@ BOOST_AUTO_TEST_CASE_TEMPLATE(simplex_copy_constructor, Simplex_tree, list_of_te std::cout << "********************************************************************" << std::endl; std::cout << "TEST OF MOVE ASSIGNMENT" << std::endl; - // A swap is a copy ctor of a tmp value, then it uses move assignment - std::swap(st3, st4); - print_simplex_filtration(st3, "First move assignment from the default Simplex_tree"); - print_simplex_filtration(st4, "Second move assignment from the default Simplex_tree"); - BOOST_CHECK(st3 == st4); - BOOST_CHECK(st == st4); - BOOST_CHECK(st3 == st); + Simplex_tree st7; + // To check there is no memory leak + st7.insert_simplex_and_subfaces({9, 10, 11}, 200.0); + st7 = std::move(st3); + print_simplex_filtration(st7, "First move assignment from the default Simplex_tree"); + Simplex_tree st8; + st8 = std::move(st4); + print_simplex_filtration(st8, "Second move assignment from the default Simplex_tree"); + + // Cross check + BOOST_CHECK(st7 == st8); + BOOST_CHECK(st == st8); + BOOST_CHECK(st7 == st); } -- cgit v1.2.3