diff options
author | vrouvrea <vrouvrea@636b058d-ea47-450e-bf9e-a15bfbe3eedb> | 2018-11-14 07:32:40 +0000 |
---|---|---|
committer | vrouvrea <vrouvrea@636b058d-ea47-450e-bf9e-a15bfbe3eedb> | 2018-11-14 07:32:40 +0000 |
commit | d121955a744c87e6b4ebbe42b42fb2321a32e9e3 (patch) | |
tree | e7ec1bf9cced8234012ed8f4aa931bc6f268bcbf | |
parent | 65d609bbf5d688552ad0f5535b6c42abffe27a1f (diff) | |
parent | bf7fcb97a34875107bf249c3d0c42cb3b1b27544 (diff) |
Merge branch simplex_tree_fix_vincent to fix issue #22 ([Simplex_tree] Rules of 5 is violated) and #23 ([Simplex_tree] shallow move constructor)
git-svn-id: svn+ssh://scm.gforge.inria.fr/svnroot/gudhi/trunk@3977 636b058d-ea47-450e-bf9e-a15bfbe3eedb
Former-commit-id: db52650107b30a99821d6b1dbc4b8d381a8b06ba
-rw-r--r-- | src/Simplex_tree/include/gudhi/Simplex_tree.h | 117 | ||||
-rw-r--r-- | src/Simplex_tree/test/CMakeLists.txt | 8 | ||||
-rw-r--r-- | src/Simplex_tree/test/simplex_tree_ctor_and_move_unit_test.cpp | 137 |
3 files changed, 244 insertions, 18 deletions
diff --git a/src/Simplex_tree/include/gudhi/Simplex_tree.h b/src/Simplex_tree/include/gudhi/Simplex_tree.h index 3339606c..13d7372c 100644 --- a/src/Simplex_tree/include/gudhi/Simplex_tree.h +++ b/src/Simplex_tree/include/gudhi/Simplex_tree.h @@ -296,12 +296,81 @@ 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_), - filtration_vect_(), - dimension_(simplex_source.dimension_) { - auto root_source = simplex_source.root_; + Simplex_tree(const Simplex_tree& complex_source) { +#ifdef DEBUG_TRACES + std::cout << "Simplex_tree copy constructor" << std::endl; +#endif // DEBUG_TRACES + copy_from(complex_source); + } + + /** \brief User-defined move constructor relocates the whole tree structure. + * \exception std::invalid_argument In debug mode, if the complex_source is invalid. + */ + Simplex_tree(Simplex_tree && complex_source) { +#ifdef DEBUG_TRACES + std::cout << "Simplex_tree move constructor" << std::endl; +#endif // DEBUG_TRACES + move_from(complex_source); + + // just need to set dimension_ on source to make it available again + // (filtration_vect_ and members are already set from the move) + complex_source.dimension_ = -1; + } + + /** \brief Destructor; deallocates the whole tree structure. */ + ~Simplex_tree() { + root_members_recursive_deletion(); + } + + /** \brief User-defined copy assignment reproduces the whole tree structure. */ + Simplex_tree& operator= (const Simplex_tree& complex_source) + { +#ifdef DEBUG_TRACES + std::cout << "Simplex_tree copy assignment" << std::endl; +#endif // DEBUG_TRACES + // Self-assignment detection + if (&complex_source != this) { + // We start by deleting root_ if not empty + root_members_recursive_deletion(); + + copy_from(complex_source); + } + return *this; + } + + /** \brief User-defined move assignment relocates the whole tree structure. + * \exception std::invalid_argument In debug mode, if the complex_source is invalid. + */ + Simplex_tree& operator=(Simplex_tree&& complex_source) + { +#ifdef DEBUG_TRACES + std::cout << "Simplex_tree move assignment" << std::endl; +#endif // DEBUG_TRACES + // Self-assignment detection + if (&complex_source != this) { + // root_ deletion in case it was not empty + root_members_recursive_deletion(); + + move_from(complex_source); + } + return *this; + } + /** @} */ // end constructor/destructor + + private: + // Copy from complex_source to "this" + void copy_from(const Simplex_tree& complex_source) { + null_vertex_ = complex_source.null_vertex_; + filtration_vect_.clear(); + dimension_ = complex_source.dimension_; + auto root_source = complex_source.root_; + + // 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()) { + map_el.second.assign_children(&root_); + } rec_copy(&root_, &root_source); } @@ -320,26 +389,38 @@ class Simplex_tree { } } - /** \brief User-defined move constructor moves the whole tree structure. */ - Simplex_tree(Simplex_tree && old) - : null_vertex_(std::move(old.null_vertex_)), - root_(std::move(old.root_)), - filtration_vect_(std::move(old.filtration_vect_)), - dimension_(std::move(old.dimension_)) { - old.dimension_ = -1; - old.root_ = Siblings(nullptr, null_vertex_); + // Move from complex_source to "this" + void move_from(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_); + + // Need to update root members (children->oncles and children need to point on the new root pointer) + for (auto& map_el : root_.members()) { + if (map_el.second.children() != &(complex_source.root_)) { + // reset children->oncles with the moved root_ pointer value + map_el.second.children()->oncles_ = &root_; + } 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")); + // and children points on root_ - to be moved + map_el.second.assign_children(&root_); + } + } } - /** \brief Destructor; deallocates the whole tree structure. */ - ~Simplex_tree() { + // delete all root_.members() recursively + void root_members_recursive_deletion() { for (auto sh = root_.members().begin(); sh != root_.members().end(); ++sh) { if (has_children(sh)) { rec_delete(sh->second.children()); } } + root_.members().clear(); } - /** @} */ // end constructor/destructor - private: + // Recursive deletion void rec_delete(Siblings * sib) { for (auto sh = sib->members().begin(); sh != sib->members().end(); ++sh) { diff --git a/src/Simplex_tree/test/CMakeLists.txt b/src/Simplex_tree/test/CMakeLists.txt index c63d8532..5bea3938 100644 --- a/src/Simplex_tree/test/CMakeLists.txt +++ b/src/Simplex_tree/test/CMakeLists.txt @@ -28,3 +28,11 @@ if (TBB_FOUND) endif() gudhi_add_coverage_test(Simplex_tree_iostream_operator_test_unit) + +add_executable ( Simplex_tree_ctor_and_move_test_unit simplex_tree_ctor_and_move_unit_test.cpp ) +target_link_libraries(Simplex_tree_ctor_and_move_test_unit ${Boost_UNIT_TEST_FRAMEWORK_LIBRARY}) +if (TBB_FOUND) + target_link_libraries(Simplex_tree_ctor_and_move_test_unit ${TBB_LIBRARIES}) +endif() + +gudhi_add_coverage_test(Simplex_tree_ctor_and_move_test_unit) 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 new file mode 100644 index 00000000..e729cf00 --- /dev/null +++ b/src/Simplex_tree/test/simplex_tree_ctor_and_move_unit_test.cpp @@ -0,0 +1,137 @@ +#include <iostream> +#include <vector> +#include <string> + +#define BOOST_TEST_DYN_LINK +#define BOOST_TEST_MODULE "simplex_tree_constructor_and_move" +#include <boost/test/unit_test.hpp> +#include <boost/mpl/list.hpp> + +// ^ +// /!\ Nothing else from Simplex_tree shall be included to test includes are well defined. +#include "gudhi/Simplex_tree.h" + +using namespace Gudhi; + +typedef boost::mpl::list<Simplex_tree<>, Simplex_tree<Simplex_tree_options_fast_persistence>> list_of_tested_variants; + +template<typename Simplex_tree> +void print_simplex_filtration(Simplex_tree& st, const std::string& msg) { + // Required before browsing through filtration values + st.initialize_filtration(); + + std::cout << "********************************************************************\n"; + std::cout << "* " << msg << "\n"; + std::cout << "* The complex contains " << st.num_simplices() << " simplices"; + std::cout << " - dimension " << st.dimension() << "\n"; + std::cout << "* Iterator on Simplices in the filtration, with [filtration value]:\n"; + for (auto f_simplex : st.filtration_simplex_range()) { + std::cout << " " + << "[" << st.filtration(f_simplex) << "] "; + for (auto vertex : st.simplex_vertex_range(f_simplex)) std::cout << "(" << vertex << ")"; + std::cout << std::endl; + } + +} + +BOOST_AUTO_TEST_CASE_TEMPLATE(simplex_copy_constructor, Simplex_tree, list_of_tested_variants) { + Simplex_tree st; + + st.insert_simplex_and_subfaces({2, 1, 0}, 3.0); + st.insert_simplex_and_subfaces({0, 1, 6, 7}, 4.0); + st.insert_simplex_and_subfaces({3, 0}, 2.0); + st.insert_simplex_and_subfaces({3, 4, 5}, 3.0); + st.insert_simplex_and_subfaces({8}, 1.0); + /* Inserted simplex: */ + /* 1 6 */ + /* o---o */ + /* /X\7/ */ + /* o---o---o---o o */ + /* 2 0 3\X/4 8 */ + /* o */ + /* 5 */ + /* */ + /* In other words: */ + /* A facet [2,1,0] */ + /* An edge [0,3] */ + /* A facet [3,4,5] */ + /* A cell [0,1,6,7] */ + /* A vertex [8] */ + + print_simplex_filtration(st, "Default Simplex_tree is initialized"); + + std::cout << "********************************************************************" << std::endl; + std::cout << "TEST OF COPY CONSTRUCTOR" << std::endl; + + Simplex_tree st1(st); + Simplex_tree st2(st); + print_simplex_filtration(st1, "First copy constructor from the default Simplex_tree"); + print_simplex_filtration(st2, "Second copy constructor from the default Simplex_tree"); + // Cross check + BOOST_CHECK(st1 == st2); + BOOST_CHECK(st == st2); + BOOST_CHECK(st1 == st); + + 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; + st4 = st; + print_simplex_filtration(st4, "Second copy assignment from the default Simplex_tree"); + + // Cross check + BOOST_CHECK(st3 == st4); + BOOST_CHECK(st == st4); + BOOST_CHECK(st3 == st); + + st = st; + print_simplex_filtration(st4, "Third self copy assignment from the default Simplex_tree"); + + BOOST_CHECK(st3 == st); + + std::cout << "********************************************************************" << std::endl; + std::cout << "TEST OF MOVE CONSTRUCTOR" << std::endl; + Simplex_tree st5(std::move(st1)); + print_simplex_filtration(st5, "First move constructor from the default Simplex_tree"); + print_simplex_filtration(st1, "First moved Simplex_tree shall be empty"); + Simplex_tree st6(std::move(st2)); + print_simplex_filtration(st6, "Second move constructor from the default Simplex_tree"); + print_simplex_filtration(st2, "Second moved Simplex_tree shall be empty"); + + // Cross check + BOOST_CHECK(st5 == st6); + BOOST_CHECK(st == st6); + BOOST_CHECK(st5 == st); + + Simplex_tree empty_st; + BOOST_CHECK(st1 == st2); + BOOST_CHECK(empty_st == st2); + BOOST_CHECK(st1 == empty_st); + + std::cout << "********************************************************************" << std::endl; + std::cout << "TEST OF MOVE ASSIGNMENT" << std::endl; + + 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); + + st = std::move(st); + print_simplex_filtration(st, "Third self move assignment from the default Simplex_tree"); + + BOOST_CHECK(st7 == st); + +} |