Merge pull request #7316 from gdamiand/CMap-bugfix-gdamiand

Use reserve(size+1) before to copy attribute

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<i>(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<i>(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<i>().reserve(amap.template attributes<i>().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<i>(amap.template get_attribute<i>(a1));
 

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;
     };
@@ -286,6 +288,13 @@ namespace CGAL {
     {
       CGAL_static_assertion_msg(Helper::template Dimension_index<i>::value>=0,
                      "copy_attribute<i> 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<Helper::template Dimension_index<i>::value>
+          (mattribute_containers).reserve
+          (std::get<Helper::template Dimension_index<i>::value>
+           (mattribute_containers).size()+1);
+
       typename Attribute_descriptor<i>::type res=
         std::get<Helper::template Dimension_index<i>::value>
         (mattribute_containers).emplace(get_attribute<i>(ah));

Combinatorial_map/include/CGAL/Compact_container_with_index.h

@@ -752,6 +752,9 @@ public:
     return false;
   }
 
+  bool owns(size_type i) const
+  { return i<capacity() && is_used(i); }
+
   bool owns_dereferenceable(const_iterator cit) const
   { return cit!=end() && owns(cit); }
 
@@ -764,7 +767,6 @@ public:
   void reserve(size_type n)
   {
     if(capacity_>=n) return;
-    capacity_=n;
     increase_size();
   }
 

Combinatorial_map/test/Combinatorial_map/CMakeLists.txt

@@ -24,6 +24,8 @@ target_compile_definitions(Combinatorial_map_copy_test_index PUBLIC USE_COMPACT_
 target_link_libraries(Combinatorial_map_copy_test_index PUBLIC CGAL CGAL::Data)
 cgal_add_compilation_test(Combinatorial_map_copy_test_index)
 
+create_single_source_cgal_program(cmap_test_split_attribute.cpp)
+
 # Link with OpenMesh if possible
 find_package(OpenMesh QUIET)
 if(TARGET OpenMesh::OpenMesh)

Combinatorial_map/test/Combinatorial_map/cmap_test_split_attribute.cpp

@@ -0,0 +1,92 @@
+#include <CGAL/Combinatorial_map.h>
+#include <CGAL/Cell_attribute.h>
+#include <vector>
+#include <algorithm>
+#include <string>
+#include <cstdlib>
+
+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<class CMap>
+  struct Dart_wrapper
+  {
+    typedef CGAL::Cell_attribute<CMap, MyInfo> attrib;
+    typedef std::tuple<void, void, attrib> Attributes;
+  };
+};
+
+struct Myitem2
+{
+  template<class CMap>
+  struct Dart_wrapper
+  {
+    typedef CGAL::Cell_attribute<CMap, MyInfo> attrib;
+    typedef std::tuple<void, void, attrib> Attributes;
+  };
+};
+
+using CMap1=CGAL::Combinatorial_map<3,Myitem1>;
+using CMap2=CGAL::Combinatorial_map<3,Myitem2>;
+
+#define NB 1000
+template<typename CMap>
+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<NB; ++i)
+  {
+    typename CMap::Dart_descriptor
+      newd=m.insert_cell_1_in_cell_2(dd, m.next(m.next(dd)));
+    if(m.template attribute<2>(newd)==CMap::null_descriptor)
+    {
+      std::cout<<"ERROR1: "<<s<<": "
+               <<"attribute<2>(newd)==CMap::null_descriptor"<<std::endl;
+      res=false;
+    }
+    else if(m.template info<2>(newd).data!=2)
+    {
+      std::cout<<"ERROR2: "<<s<<": "<<m.template info<2>(newd).data<<std::endl;
+      res=false;
+    }
+
+    newd=m.template opposite<2>(newd);
+    if(m.template attribute<2>(newd)==CMap::null_descriptor)
+    {
+      std::cout<<"ERROR3: "<<s<<": "
+               <<"attribute<2>(newd)==CMap::null_descriptor"<<std::endl;
+      res=false;
+    }
+    else if(m.template info<2>(newd).data!=2)
+    {
+      std::cout<<"ERROR4: "<<s<<": "<<m.template info<2>(newd).data<<std::endl;
+      res=false;
+    }
+  }
+  return res;
+}
+
+int main()
+{
+  if(!test<CMap1>("CMap1") || !test<CMap2>("CMap2"))
+  { return EXIT_FAILURE; }
+
+  return EXIT_SUCCESS;
+}

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<i>(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<i>(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<i>().reserve(amap.template attributes<i>().size()+1);
+
     Attribute_descriptor_i a2 = amap.template
       create_attribute<i>(amap.template get_attribute<i>(a1));
 

Generalized_map/include/CGAL/Generalized_map_storages_with_index.h

@@ -242,6 +242,13 @@ namespace CGAL {
     {
       CGAL_static_assertion_msg(Helper::template Dimension_index<i>::value>=0,
                      "copy_attribute<i> 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<Helper::template Dimension_index<i>::value>
+          (mattribute_containers).reserve
+          (std::get<Helper::template Dimension_index<i>::value>
+           (mattribute_containers).size()+1);
+
       typename Attribute_descriptor<i>::type res=
         std::get<Helper::template Dimension_index<i>::value>
         (mattribute_containers).emplace(get_attribute<i>(ah));

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")

Generalized_map/test/Generalized_map/gmap_test_split_attribute.cpp

@@ -0,0 +1,92 @@
+#include <CGAL/Generalized_map.h>
+#include <CGAL/Cell_attribute.h>
+#include <vector>
+#include <algorithm>
+#include <string>
+#include <cstdlib>
+
+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<class GMap>
+  struct Dart_wrapper
+  {
+    typedef CGAL::Cell_attribute<GMap, MyInfo> attrib;
+    typedef std::tuple<void, void, attrib> Attributes;
+  };
+};
+
+struct Myitem2
+{
+  template<class GMap>
+  struct Dart_wrapper
+  {
+    typedef CGAL::Cell_attribute<GMap, MyInfo> attrib;
+    typedef std::tuple<void, void, attrib> Attributes;
+  };
+};
+
+using GMap1=CGAL::Generalized_map<3,Myitem1>;
+using GMap2=CGAL::Generalized_map<3,Myitem2>;
+
+#define NB 1000
+template<typename GMap>
+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<NB; ++i)
+  {
+    typename GMap::Dart_descriptor
+      newd=m.insert_cell_1_in_cell_2(dd, m.template alpha<0,1,0>(dd));
+    if(m.template attribute<2>(newd)==GMap::null_descriptor)
+    {
+      std::cout<<"ERROR1: "<<s<<": "
+               <<"attribute<2>(newd)==GMap::null_descriptor"<<std::endl;
+      res=false;
+    }
+    else if(m.template info<2>(newd).data!=2)
+    {
+      std::cout<<"ERROR2: "<<s<<": "<<m.template info<2>(newd).data<<std::endl;
+      res=false;
+    }
+
+    newd=m.template opposite<2>(newd);
+    if(m.template attribute<2>(newd)==GMap::null_descriptor)
+    {
+      std::cout<<"ERROR3: "<<s<<": "
+               <<"attribute<2>(newd)==GMap::null_descriptor"<<std::endl;
+      res=false;
+    }
+    else if(m.template info<2>(newd).data!=2)
+    {
+      std::cout<<"ERROR4: "<<s<<": "<<m.template info<2>(newd).data<<std::endl;
+      res=false;
+    }
+  }
+  return res;
+}
+
+int main()
+{
+  if(!test<GMap1>("GMap1") || !test<GMap2>("GMap2"))
+  { return EXIT_FAILURE; }
+
+  return EXIT_SUCCESS;
+}

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;
     };
@@ -307,6 +309,13 @@ namespace CGAL {
     {
       CGAL_static_assertion_msg(Helper::template Dimension_index<i>::value>=0,
                      "copy_attribute<i> 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<Helper::template Dimension_index<i>::value>
+          (mattribute_containers).reserve
+          (std::get<Helper::template Dimension_index<i>::value>
+           (mattribute_containers).size()+1);
+
       typename Attribute_descriptor<i>::type res=
         std::get<Helper::template Dimension_index<i>::value>
         (mattribute_containers).emplace(get_attribute<i>(ah));

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<i>::value>=0,
                      "copy_attribute<i> 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<Helper::template Dimension_index<i>::value>
+          (mattribute_containers).reserve
+          (std::get<Helper::template Dimension_index<i>::value>
+           (mattribute_containers).size()+1);
+
       typename Attribute_descriptor<i>::type res=
         std::get<Helper::template Dimension_index<i>::value>
         (mattribute_containers).emplace(get_attribute<i>(ah));