From 0189b6b6b2c45171574063dbb81ca9a464b78da3 Mon Sep 17 00:00:00 2001 From: Guillaume Damiand Date: Thu, 9 Mar 2023 17:07:36 +0100 Subject: [PATCH 01/11] Add a copy constructor in Cell_attribute. This solves a bug in one of my prog, which occurs only for CMap with index. I don't understand why the default copy constructor is not enough for CMap with index (while it works for CMap with handle). --- Combinatorial_map/include/CGAL/Cell_attribute.h | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/Combinatorial_map/include/CGAL/Cell_attribute.h b/Combinatorial_map/include/CGAL/Cell_attribute.h index 0c931f0e30d..da3c9764fa6 100644 --- a/Combinatorial_map/include/CGAL/Cell_attribute.h +++ b/Combinatorial_map/include/CGAL/Cell_attribute.h @@ -397,6 +397,11 @@ struct Init_id; typedef OnSplit On_split; typedef void Info; + // Copy constructor why this is required for CMap with index and not for CMap with handle ? + Cell_attribute(const Cell_attribute& other) : + Cell_attribute_without_info(other) + {} + protected: /// Default constructor. Cell_attribute() @@ -460,6 +465,12 @@ struct Init_id; bool operator!=(const Self& other) const { return !operator==(other); } + // Copy constructor why this is required for CMap with index and not cor CMap with handle ? + Cell_attribute(const Self& other) : + Cell_attribute_without_info(other), + Info_for_cell_attribute(other) + {} + protected: /// Default constructor. Cell_attribute() From 4ad06f887212f3f343a63177f734ebc6a78a85bf Mon Sep 17 00:00:00 2001 From: Guillaume Damiand Date: Thu, 9 Mar 2023 20:23:53 +0100 Subject: [PATCH 02/11] add an example with a bug for cmap with index --- .../examples/Combinatorial_map/map_3_bug.cpp | 67 +++++++++++++++++++ 1 file changed, 67 insertions(+) create mode 100644 Combinatorial_map/examples/Combinatorial_map/map_3_bug.cpp diff --git a/Combinatorial_map/examples/Combinatorial_map/map_3_bug.cpp b/Combinatorial_map/examples/Combinatorial_map/map_3_bug.cpp new file mode 100644 index 00000000000..49a8ddf0d35 --- /dev/null +++ b/Combinatorial_map/examples/Combinatorial_map/map_3_bug.cpp @@ -0,0 +1,67 @@ +#include +#include +#include +#include +#include +#include + +struct MyInfo +{ + MyInfo() :data(1) + {} + + MyInfo(int i) :data(i) + {} + + int data; +}; + +struct Myitem1 +{ + using Use_index=CGAL::Tag_true; // use indices + using Index_type=std::uint16_t; // 16 bits + template + struct Dart_wrapper + { + typedef CGAL::Cell_attribute attrib; + typedef std::tuple Attributes; + }; +}; + +struct Myitem2 +{ + template + struct Dart_wrapper + { + typedef CGAL::Cell_attribute attrib; + typedef std::tuple Attributes; + }; +}; + +using CMap1=CGAL::Combinatorial_map<3,Myitem1>; +using CMap2=CGAL::Combinatorial_map<3,Myitem2>; + +#define NB 100 +int main() +{ + CMap1 cm1; + CMap2 cm2; + + CMap1::Attribute_descriptor<0>::type a1=cm1.create_attribute<0>(2); + for(std::size_t i=0; i::type a2=cm1.create_attribute<0>(cm1.info_of_attribute<0>(a1)); + if(cm1.info_of_attribute<0>(a1).data!=cm1.info_of_attribute<0>(a2).data) + { std::cout<<"ERROR1: "<(a1).data<<" and "<(a2).data<<" ("<::type a3=cm2.create_attribute<0>(3); + for(std::size_t i=0; i::type a4=cm2.create_attribute<0>(cm2.info_of_attribute<0>(a3)); + if(cm2.info_of_attribute<0>(a3).data!=cm2.info_of_attribute<0>(a4).data) + { std::cout<<"ERROR2: "<(a3).data<<" and "<(a4).data<<" ("< Date: Fri, 10 Mar 2023 10:44:20 +0100 Subject: [PATCH 03/11] Remove copy constructors in cell attribute (added in the last commit but not required) --- Combinatorial_map/include/CGAL/Cell_attribute.h | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/Combinatorial_map/include/CGAL/Cell_attribute.h b/Combinatorial_map/include/CGAL/Cell_attribute.h index da3c9764fa6..df2709de2f7 100644 --- a/Combinatorial_map/include/CGAL/Cell_attribute.h +++ b/Combinatorial_map/include/CGAL/Cell_attribute.h @@ -397,12 +397,6 @@ struct Init_id; typedef OnSplit On_split; typedef void Info; - // Copy constructor why this is required for CMap with index and not for CMap with handle ? - Cell_attribute(const Cell_attribute& other) : - Cell_attribute_without_info(other) - {} - - protected: /// Default constructor. Cell_attribute() {} @@ -465,12 +459,6 @@ struct Init_id; bool operator!=(const Self& other) const { return !operator==(other); } - // Copy constructor why this is required for CMap with index and not cor CMap with handle ? - Cell_attribute(const Self& other) : - Cell_attribute_without_info(other), - Info_for_cell_attribute(other) - {} - protected: /// Default constructor. Cell_attribute() From 83ae1d54ea60cc1c79dfde7f7c7dbc2a0fa49c12 Mon Sep 17 00:00:00 2001 From: Guillaume Damiand Date: Fri, 10 Mar 2023 10:46:23 +0100 Subject: [PATCH 04/11] add missing own method for with index cases --- .../include/CGAL/Combinatorial_map_storages_with_index.h | 2 ++ .../include/CGAL/CMap_linear_cell_complex_storages_with_index.h | 2 ++ 2 files changed, 4 insertions(+) diff --git a/Combinatorial_map/include/CGAL/Combinatorial_map_storages_with_index.h b/Combinatorial_map/include/CGAL/Combinatorial_map_storages_with_index.h index aad7d643787..5b4bfbc0f7a 100644 --- a/Combinatorial_map/include/CGAL/Combinatorial_map_storages_with_index.h +++ b/Combinatorial_map/include/CGAL/Combinatorial_map_storages_with_index.h @@ -146,6 +146,8 @@ namespace CGAL { { return cit; } bool is_used(size_type i) const { return mmap.mdarts.is_used(i); } + bool owns(size_type i) const + { return mmap.mdarts.owns(i); } private: Self & mmap; }; diff --git a/Linear_cell_complex/include/CGAL/CMap_linear_cell_complex_storages_with_index.h b/Linear_cell_complex/include/CGAL/CMap_linear_cell_complex_storages_with_index.h index cde16d37cf4..e17851e096a 100644 --- a/Linear_cell_complex/include/CGAL/CMap_linear_cell_complex_storages_with_index.h +++ b/Linear_cell_complex/include/CGAL/CMap_linear_cell_complex_storages_with_index.h @@ -167,6 +167,8 @@ namespace CGAL { { return cit; } bool is_used(size_type i) const { return mmap.mdarts.is_used(i); } + bool owns(size_type i) const + { return mmap.mdarts.owns(i); } private: Self & mmap; }; From 477f71b876358ae7094f19017d62028ae5f8bfce Mon Sep 17 00:00:00 2001 From: Guillaume Damiand Date: Fri, 10 Mar 2023 10:47:01 +0100 Subject: [PATCH 05/11] Add missing own method for cc with index --- Combinatorial_map/include/CGAL/Compact_container_with_index.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/Combinatorial_map/include/CGAL/Compact_container_with_index.h b/Combinatorial_map/include/CGAL/Compact_container_with_index.h index a87122500fc..abb1739303f 100644 --- a/Combinatorial_map/include/CGAL/Compact_container_with_index.h +++ b/Combinatorial_map/include/CGAL/Compact_container_with_index.h @@ -752,6 +752,9 @@ public: return false; } + bool owns(size_type i) const + { return i Date: Fri, 10 Mar 2023 11:30:07 +0100 Subject: [PATCH 06/11] Add reserve before the creation of a new attribute, copy of an old one: solve a bug for the index version of CMap/GMap when the underlying vector is reallocated --- .../internal/Combinatorial_map_group_functors.h | 9 +++++++++ .../internal/Generalized_map_group_functors.h | 7 +++++++ 2 files changed, 16 insertions(+) diff --git a/Combinatorial_map/include/CGAL/Combinatorial_map/internal/Combinatorial_map_group_functors.h b/Combinatorial_map/include/CGAL/Combinatorial_map/internal/Combinatorial_map_group_functors.h index 1db4ec2cbda..06f3c4f4dc3 100644 --- a/Combinatorial_map/include/CGAL/Combinatorial_map/internal/Combinatorial_map_group_functors.h +++ b/Combinatorial_map/include/CGAL/Combinatorial_map/internal/Combinatorial_map_group_functors.h @@ -566,6 +566,15 @@ void test_split_attribute_functor_one_dart Attribute_descriptor_i a1 = amap.template attribute(adart); if ( found_attributes.is_defined(a1) ) { // Here the attribute was already present in the hash_map + + // We need to call reserve for the cc with index case. Indeed, if the vector + // is reallocated, the reference returned by get_attribute(a1) will be + // invalidated, and the copy will be wrong. Note that there is no overhead + // since the creation of the attribute need one allocation. + amap.template attributes().reserve(amap.template attributes().size()+1); + + // Now we are sure that the creation of a new attribute will not imply + // a realloc. Attribute_descriptor_i a2 = amap.template create_attribute(amap.template get_attribute(a1)); diff --git a/Generalized_map/include/CGAL/Generalized_map/internal/Generalized_map_group_functors.h b/Generalized_map/include/CGAL/Generalized_map/internal/Generalized_map_group_functors.h index cbde9898096..4a15c8ea733 100644 --- a/Generalized_map/include/CGAL/Generalized_map/internal/Generalized_map_group_functors.h +++ b/Generalized_map/include/CGAL/Generalized_map/internal/Generalized_map_group_functors.h @@ -293,6 +293,13 @@ void GMap_test_split_attribute_functor_one_dart Attribute_descriptor_i a1 = amap.template attribute(adart); if ( found_attributes.is_defined(a1) ) { // Here the attribute was already present in the hash_map + + // We need to call reserve for the cc with index case. Indeed, if the vector + // is reallocated, the reference returned by get_attribute(a1) will be + // invalidated, and the copy will be wrong. Note that there is no overhead + // since the creation of the attribute need one allocation. + amap.template attributes().reserve(amap.template attributes().size()+1); + Attribute_descriptor_i a2 = amap.template create_attribute(amap.template get_attribute(a1)); From 8dc54d3c62c90e37d6f46e86a256271bd3aa26f1 Mon Sep 17 00:00:00 2001 From: Guillaume Damiand Date: Fri, 10 Mar 2023 13:20:03 +0100 Subject: [PATCH 07/11] Add a reserve before to copy attribute (to solve a bug for index versions) --- .../include/CGAL/Combinatorial_map_storages_with_index.h | 7 +++++++ .../include/CGAL/Generalized_map_storages_with_index.h | 7 +++++++ 2 files changed, 14 insertions(+) diff --git a/Combinatorial_map/include/CGAL/Combinatorial_map_storages_with_index.h b/Combinatorial_map/include/CGAL/Combinatorial_map_storages_with_index.h index 5b4bfbc0f7a..584442412c5 100644 --- a/Combinatorial_map/include/CGAL/Combinatorial_map_storages_with_index.h +++ b/Combinatorial_map/include/CGAL/Combinatorial_map_storages_with_index.h @@ -288,6 +288,13 @@ namespace CGAL { { CGAL_static_assertion_msg(Helper::template Dimension_index::value>=0, "copy_attribute called but i-attributes are disabled."); + // We need to do a reserve before the emplace in order to avoid a bug of + // invalid reference when the container is reallocated. + std::get::value> + (mattribute_containers).reserve + (std::get::value> + (mattribute_containers).size()+1); + typename Attribute_descriptor::type res= std::get::value> (mattribute_containers).emplace(get_attribute(ah)); diff --git a/Generalized_map/include/CGAL/Generalized_map_storages_with_index.h b/Generalized_map/include/CGAL/Generalized_map_storages_with_index.h index 80044bdc7d3..828004b8416 100644 --- a/Generalized_map/include/CGAL/Generalized_map_storages_with_index.h +++ b/Generalized_map/include/CGAL/Generalized_map_storages_with_index.h @@ -242,6 +242,13 @@ namespace CGAL { { CGAL_static_assertion_msg(Helper::template Dimension_index::value>=0, "copy_attribute called but i-attributes are disabled."); + // We need to do a reserve before the emplace in order to avoid a bug of + // invalid reference when the container is reallocated. + std::get::value> + (mattribute_containers).reserve + (std::get::value> + (mattribute_containers).size()+1); + typename Attribute_descriptor::type res= std::get::value> (mattribute_containers).emplace(get_attribute(ah)); From 52d78763d61c2adcd04def484ff8587efe2a6dbd Mon Sep 17 00:00:00 2001 From: Guillaume Damiand Date: Fri, 10 Mar 2023 13:25:44 +0100 Subject: [PATCH 08/11] Bug fix in cc with index --- Combinatorial_map/include/CGAL/Compact_container_with_index.h | 1 - 1 file changed, 1 deletion(-) diff --git a/Combinatorial_map/include/CGAL/Compact_container_with_index.h b/Combinatorial_map/include/CGAL/Compact_container_with_index.h index abb1739303f..044db27dc87 100644 --- a/Combinatorial_map/include/CGAL/Compact_container_with_index.h +++ b/Combinatorial_map/include/CGAL/Compact_container_with_index.h @@ -767,7 +767,6 @@ public: void reserve(size_type n) { if(capacity_>=n) return; - capacity_=n; increase_size(); } From 46a35ff64222e6230265e7b252e980d0b01fd737 Mon Sep 17 00:00:00 2001 From: Guillaume Damiand Date: Fri, 10 Mar 2023 13:26:23 +0100 Subject: [PATCH 09/11] Add a test for copy attributes for cmap/gmap, with and without indices. --- .../examples/Combinatorial_map/map_3_bug.cpp | 67 -------------- .../test/Combinatorial_map/CMakeLists.txt | 2 + .../cmap_test_split_attribute.cpp | 92 +++++++++++++++++++ .../test/Generalized_map/CMakeLists.txt | 2 + .../gmap_test_split_attribute.cpp | 92 +++++++++++++++++++ 5 files changed, 188 insertions(+), 67 deletions(-) delete mode 100644 Combinatorial_map/examples/Combinatorial_map/map_3_bug.cpp create mode 100644 Combinatorial_map/test/Combinatorial_map/cmap_test_split_attribute.cpp create mode 100644 Generalized_map/test/Generalized_map/gmap_test_split_attribute.cpp diff --git a/Combinatorial_map/examples/Combinatorial_map/map_3_bug.cpp b/Combinatorial_map/examples/Combinatorial_map/map_3_bug.cpp deleted file mode 100644 index 49a8ddf0d35..00000000000 --- a/Combinatorial_map/examples/Combinatorial_map/map_3_bug.cpp +++ /dev/null @@ -1,67 +0,0 @@ -#include -#include -#include -#include -#include -#include - -struct MyInfo -{ - MyInfo() :data(1) - {} - - MyInfo(int i) :data(i) - {} - - int data; -}; - -struct Myitem1 -{ - using Use_index=CGAL::Tag_true; // use indices - using Index_type=std::uint16_t; // 16 bits - template - struct Dart_wrapper - { - typedef CGAL::Cell_attribute attrib; - typedef std::tuple Attributes; - }; -}; - -struct Myitem2 -{ - template - struct Dart_wrapper - { - typedef CGAL::Cell_attribute attrib; - typedef std::tuple Attributes; - }; -}; - -using CMap1=CGAL::Combinatorial_map<3,Myitem1>; -using CMap2=CGAL::Combinatorial_map<3,Myitem2>; - -#define NB 100 -int main() -{ - CMap1 cm1; - CMap2 cm2; - - CMap1::Attribute_descriptor<0>::type a1=cm1.create_attribute<0>(2); - for(std::size_t i=0; i::type a2=cm1.create_attribute<0>(cm1.info_of_attribute<0>(a1)); - if(cm1.info_of_attribute<0>(a1).data!=cm1.info_of_attribute<0>(a2).data) - { std::cout<<"ERROR1: "<(a1).data<<" and "<(a2).data<<" ("<::type a3=cm2.create_attribute<0>(3); - for(std::size_t i=0; i::type a4=cm2.create_attribute<0>(cm2.info_of_attribute<0>(a3)); - if(cm2.info_of_attribute<0>(a3).data!=cm2.info_of_attribute<0>(a4).data) - { std::cout<<"ERROR2: "<(a3).data<<" and "<(a4).data<<" ("< +#include +#include +#include +#include +#include + +struct MyInfo +{ + MyInfo() :data(1) + {} + + MyInfo(int i) :data(i) + {} + + int data; +}; + +struct Myitem1 +{ + using Use_index=CGAL::Tag_true; // use indices + using Index_type=std::uint16_t; // 16 bits + template + struct Dart_wrapper + { + typedef CGAL::Cell_attribute attrib; + typedef std::tuple Attributes; + }; +}; + +struct Myitem2 +{ + template + struct Dart_wrapper + { + typedef CGAL::Cell_attribute attrib; + typedef std::tuple Attributes; + }; +}; + +using CMap1=CGAL::Combinatorial_map<3,Myitem1>; +using CMap2=CGAL::Combinatorial_map<3,Myitem2>; + +#define NB 1000 +template +bool test(const std::string& s) +{ + bool res=true; + CMap m; + // 1) create a face and one attribute. + typename CMap::Dart_descriptor dd=m.make_combinatorial_polygon(4); + m.template set_attribute<2>(dd, m.template create_attribute<2>(2)); + // 2) Split this face NB times => will create new 2-attributes for new faces + for(std::size_t i=0; i(newd)==CMap::null_descriptor) + { + std::cout<<"ERROR1: "<(newd)==CMap::null_descriptor"<(newd).data!=2) + { + std::cout<<"ERROR2: "<(newd).data<(newd); + if(m.template attribute<2>(newd)==CMap::null_descriptor) + { + std::cout<<"ERROR3: "<(newd)==CMap::null_descriptor"<(newd).data!=2) + { + std::cout<<"ERROR4: "<(newd).data<("CMap1") || !test("CMap2")) + { return EXIT_FAILURE; } + + return EXIT_SUCCESS; +} diff --git a/Generalized_map/test/Generalized_map/CMakeLists.txt b/Generalized_map/test/Generalized_map/CMakeLists.txt index 642815d6c08..e773e5c3218 100644 --- a/Generalized_map/test/Generalized_map/CMakeLists.txt +++ b/Generalized_map/test/Generalized_map/CMakeLists.txt @@ -18,3 +18,5 @@ add_executable(Generalized_map_test_index Generalized_map_test.cpp ${hfiles}) target_compile_definitions(Generalized_map_test_index PUBLIC USE_COMPACT_CONTAINER_WITH_INDEX) target_link_libraries(Generalized_map_test_index PUBLIC CGAL CGAL::Data) cgal_add_compilation_test(Generalized_map_test_index) + +create_single_source_cgal_program("gmap_test_split_attribute.cpp") diff --git a/Generalized_map/test/Generalized_map/gmap_test_split_attribute.cpp b/Generalized_map/test/Generalized_map/gmap_test_split_attribute.cpp new file mode 100644 index 00000000000..51e5618ce9c --- /dev/null +++ b/Generalized_map/test/Generalized_map/gmap_test_split_attribute.cpp @@ -0,0 +1,92 @@ +#include +#include +#include +#include +#include +#include + +struct MyInfo +{ + MyInfo() :data(1) + {} + + MyInfo(int i) :data(i) + {} + + int data; +}; + +struct Myitem1 +{ + using Use_index=CGAL::Tag_true; // use indices + using Index_type=std::uint16_t; // 16 bits + template + struct Dart_wrapper + { + typedef CGAL::Cell_attribute attrib; + typedef std::tuple Attributes; + }; +}; + +struct Myitem2 +{ + template + struct Dart_wrapper + { + typedef CGAL::Cell_attribute attrib; + typedef std::tuple Attributes; + }; +}; + +using GMap1=CGAL::Generalized_map<3,Myitem1>; +using GMap2=CGAL::Generalized_map<3,Myitem2>; + +#define NB 1000 +template +bool test(const std::string& s) +{ + bool res=true; + GMap m; + // 1) create a face and one attribute. + typename GMap::Dart_descriptor dd=m.make_combinatorial_polygon(4); + m.template set_attribute<2>(dd, m.template create_attribute<2>(2)); + // 2) Split this face NB times => will create new 2-attributes for new faces + for(std::size_t i=0; i(dd)); + if(m.template attribute<2>(newd)==GMap::null_descriptor) + { + std::cout<<"ERROR1: "<(newd)==GMap::null_descriptor"<(newd).data!=2) + { + std::cout<<"ERROR2: "<(newd).data<(newd); + if(m.template attribute<2>(newd)==GMap::null_descriptor) + { + std::cout<<"ERROR3: "<(newd)==GMap::null_descriptor"<(newd).data!=2) + { + std::cout<<"ERROR4: "<(newd).data<("GMap1") || !test("GMap2")) + { return EXIT_FAILURE; } + + return EXIT_SUCCESS; +} From 804f28804df1b2f466869b330fa3de3f408e50fc Mon Sep 17 00:00:00 2001 From: Guillaume Damiand Date: Fri, 10 Mar 2023 13:31:56 +0100 Subject: [PATCH 10/11] same modif for lcc --- .../CGAL/CMap_linear_cell_complex_storages_with_index.h | 7 +++++++ .../CGAL/GMap_linear_cell_complex_storages_with_index.h | 7 +++++++ 2 files changed, 14 insertions(+) diff --git a/Linear_cell_complex/include/CGAL/CMap_linear_cell_complex_storages_with_index.h b/Linear_cell_complex/include/CGAL/CMap_linear_cell_complex_storages_with_index.h index e17851e096a..8417e20b226 100644 --- a/Linear_cell_complex/include/CGAL/CMap_linear_cell_complex_storages_with_index.h +++ b/Linear_cell_complex/include/CGAL/CMap_linear_cell_complex_storages_with_index.h @@ -309,6 +309,13 @@ namespace CGAL { { CGAL_static_assertion_msg(Helper::template Dimension_index::value>=0, "copy_attribute called but i-attributes are disabled."); + // We need to do a reserve before the emplace in order to avoid a bug of + // invalid reference when the container is reallocated. + std::get::value> + (mattribute_containers).reserve + (std::get::value> + (mattribute_containers).size()+1); + typename Attribute_descriptor::type res= std::get::value> (mattribute_containers).emplace(get_attribute(ah)); diff --git a/Linear_cell_complex/include/CGAL/GMap_linear_cell_complex_storages_with_index.h b/Linear_cell_complex/include/CGAL/GMap_linear_cell_complex_storages_with_index.h index a2a58088828..27d58a7651a 100644 --- a/Linear_cell_complex/include/CGAL/GMap_linear_cell_complex_storages_with_index.h +++ b/Linear_cell_complex/include/CGAL/GMap_linear_cell_complex_storages_with_index.h @@ -261,6 +261,13 @@ namespace CGAL { { CGAL_static_assertion_msg(Helper::template Dimension_index::value>=0, "copy_attribute called but i-attributes are disabled."); + // We need to do a reserve before the emplace in order to avoid a bug of + // invalid reference when the container is reallocated. + std::get::value> + (mattribute_containers).reserve + (std::get::value> + (mattribute_containers).size()+1); + typename Attribute_descriptor::type res= std::get::value> (mattribute_containers).emplace(get_attribute(ah)); From da59938748f5fe8e4aa261fa6687f12ebf753022 Mon Sep 17 00:00:00 2001 From: Guillaume Damiand Date: Fri, 10 Mar 2023 13:33:31 +0100 Subject: [PATCH 11/11] readd protected removed by mistake --- Combinatorial_map/include/CGAL/Cell_attribute.h | 1 + 1 file changed, 1 insertion(+) diff --git a/Combinatorial_map/include/CGAL/Cell_attribute.h b/Combinatorial_map/include/CGAL/Cell_attribute.h index df2709de2f7..0c931f0e30d 100644 --- a/Combinatorial_map/include/CGAL/Cell_attribute.h +++ b/Combinatorial_map/include/CGAL/Cell_attribute.h @@ -397,6 +397,7 @@ struct Init_id; typedef OnSplit On_split; typedef void Info; + protected: /// Default constructor. Cell_attribute() {}