fixed assignment bug with POCXA, fancy, unequal allocators (#214)

include/boost/unordered/detail/foa/concurrent_table.hpp

@@ -504,16 +504,13 @@ public:
           x.arrays.elements_});},
       size_ctrl_type{x.size_ctrl.ml,x.size_ctrl.size}}
   {
-    ah.release();
+    x.arrays=ah.release();
-    x.arrays=ah.get();
     x.size_ctrl.ml=x.initial_max_load();
     x.size_ctrl.size=0;
   }
 
   concurrent_table(compatible_nonconcurrent_table&& x):
-    concurrent_table(std::move(x), arrays_holder<
+    concurrent_table(std::move(x),x.make_empty_arrays())
-      typename compatible_nonconcurrent_table::arrays_type,Allocator
-    >{compatible_nonconcurrent_table::arrays_type::new_(x.al(),0),x.al()})
   {}
 
   ~concurrent_table()=default;

include/boost/unordered/detail/foa/core.hpp

@@ -970,28 +970,20 @@ Ptr to_pointer(Ptr p)
 template<typename Arrays,typename Allocator>
 struct arrays_holder
 {
-  arrays_holder(Arrays const& arrays, Allocator const& al)
+  arrays_holder(const Arrays& arrays,const Allocator& al):
-    :arrays_{arrays},al_{al}
+    arrays_{arrays},al_{al}
   {}
   
-  arrays_holder(arrays_holder const&)=delete;
+  /* not defined but VS in pre-C++17 mode needs to see it for RVO */
+  arrays_holder(arrays_holder const&);
   arrays_holder& operator=(arrays_holder const&)=delete;
 
-  ~arrays_holder()
+  ~arrays_holder(){if(!released_)arrays_.delete_(al_,arrays_);}
-  {
-    if (!released_){
-      arrays_.delete_(al_,arrays_);
-    }
-  }
 
-  Arrays const& get()const
+  const Arrays& release()
-  {
-    return arrays_;
-  }
-
-  void release()
   {
     released_=true;
+    return arrays_;
   }
 
 private:
@@ -1367,6 +1359,7 @@ public:
   using size_type=std::size_t;
   using difference_type=std::ptrdiff_t;
   using locator=table_locator<group_type,element_type>;
+  using arrays_holder_type=arrays_holder<arrays_type,Allocator>;
 
   table_core(
     std::size_t n=default_bucket_count,const Hash& h_=Hash(),
@@ -1394,15 +1387,12 @@ public:
     table_core{x,alloc_traits::select_on_container_copy_construction(x.al())}{}
 
   template<typename ArraysFn>
-  table_core(
+  table_core(table_core&& x,arrays_holder_type&& ah,ArraysFn arrays_fn):
-    table_core&& x,arrays_holder<arrays_type,Allocator>&& ah,
-    ArraysFn arrays_fn):
     table_core(
       std::move(x.h()),std::move(x.pred()),std::move(x.al()),
       arrays_fn,x.size_ctrl)
   {
-    ah.release();
+    x.arrays=ah.release();
-    x.arrays=ah.get();
     x.size_ctrl.ml=x.initial_max_load();
     x.size_ctrl.size=0;
   }
@@ -1414,9 +1404,7 @@ public:
       std::is_nothrow_move_constructible<Allocator>::value&&
       !uses_fancy_pointers):
     table_core{
-      std::move(x),arrays_holder<arrays_type,Allocator>{
+      std::move(x),x.make_empty_arrays(),[&x]{return x.arrays;}}
-        x.new_arrays(0),x.al()},
-      [&x]{return x.arrays;}}
   {}
 
   table_core(const table_core& x,const Allocator& al_):
@@ -1468,6 +1456,11 @@ public:
     }
   }
 
+  arrays_holder_type make_empty_arrays()const
+  {
+    return make_arrays(0);
+  }
+
   table_core& operator=(const table_core& x)
   {
     BOOST_UNORDERED_STATIC_ASSERT_HASH_PRED(Hash, Pred)
@@ -1482,9 +1475,6 @@ public:
       hasher    tmp_h=x.h();
       key_equal tmp_p=x.pred();
 
-      /* already noexcept, clear() before we swap the Hash, Pred just in case
-       * the clear() impl relies on them at some point in the future.
-       */
       clear();
 
       /* Because we've asserted at compile-time that Hash and Pred are nothrow
@@ -1496,7 +1486,12 @@ public:
       swap(pred(),tmp_p);
 
       if_constexpr<pocca>([&,this]{
-        if(al()!=x.al())reserve(0);
+        if(al()!=x.al()){
+          auto ah=x.make_arrays(std::size_t(std::ceil(float(x.size())/mlf)));
+          delete_arrays(arrays);
+          arrays=ah.release();
+          size_ctrl.ml=initial_max_load();
+        }
         copy_assign_if<pocca>(al(),x.al());
       });
       /* noshrink: favor memory reuse over tightness */
@@ -1535,16 +1530,24 @@ public:
       using std::swap;
 
       clear();
+
+      if(pocma||al()==x.al()){
+        auto ah=x.make_empty_arrays();
+        swap(h(),x.h());
+        swap(pred(),x.pred());
+        delete_arrays(arrays);
+        move_assign_if<pocma>(al(),x.al());
+        arrays=x.arrays;
+        size_ctrl.ml=std::size_t(x.size_ctrl.ml);
+        size_ctrl.size=std::size_t(x.size_ctrl.size);
+        x.arrays=ah.release();
+        x.size_ctrl.ml=x.initial_max_load();
+        x.size_ctrl.size=0;
+      }
+      else{
         swap(h(),x.h());
         swap(pred(),x.pred());
 
-      if(pocma||al()==x.al()){
-        reserve(0);
-        move_assign_if<pocma>(al(),x.al());
-        swap(arrays,x.arrays);
-        swap(size_ctrl,x.size_ctrl);
-      }
-      else{
         /* noshrink: favor memory reuse over tightness */
         noshrink_reserve(x.size());
         clear_on_exit c{x};
@@ -1940,12 +1943,12 @@ private:
   {
   }
 
-  arrays_type new_arrays(std::size_t n)
+  arrays_type new_arrays(std::size_t n)const
   {
     return arrays_type::new_(al(),n);
   }
 
-  arrays_type new_arrays_for_growth()
+  arrays_type new_arrays_for_growth()const
   {
     /* Due to the anti-drift mechanism (see recover_slot), the new arrays may
      * be of the same size as the old arrays; in the limit, erasing one
@@ -1966,6 +1969,11 @@ private:
     arrays_type::delete_(al(),arrays_);
   }
 
+  arrays_holder_type make_arrays(std::size_t n)const
+  {
+    return {new_arrays(n),al()};
+  }
+
   template<typename Key,typename... Args>
   void construct_element_from_try_emplace_args(
     element_type* p,std::false_type,Key&& x,Args&&... args)

include/boost/unordered/detail/foa/table.hpp

@@ -555,19 +555,15 @@ private:
         x.arrays.elements_};},
       size_ctrl_type{x.size_ctrl.ml,x.size_ctrl.size}}
   {
-    ah.release();
-
     compatible_concurrent_table::arrays_type::delete_group_access(x.al(),x.arrays);
-    x.arrays=ah.get();
+    x.arrays=ah.release();
     x.size_ctrl.ml=x.initial_max_load();
     x.size_ctrl.size=0;
   }
 
   template<typename ExclusiveLockGuard>
   table(compatible_concurrent_table&& x,ExclusiveLockGuard):
-    table(std::move(x),arrays_holder<
+    table(std::move(x),x.make_empty_arrays())
-      typename compatible_concurrent_table::arrays_type,Allocator
-    >{compatible_concurrent_table::arrays_type::new_(x.al(),0),x.al()})
   {}
 
   struct erase_on_exit

test/cfoa/assign_tests.cpp

@@ -104,78 +104,82 @@ std::initializer_list<set_type::value_type> set_init_list{
 auto test_map_and_init_list=std::make_pair(test_map,map_init_list);
 auto test_set_and_init_list=std::make_pair(test_set,set_init_list);
 
-template <class T> struct pocca_allocator
+template <class T,bool POCCA, bool POCMA>
+struct poca_allocator: fancy_allocator<T>
 {
-  using propagate_on_container_copy_assignment = std::true_type;
+  using super = fancy_allocator<T>;
+  using pointer = typename super::pointer;
+  using propagate_on_container_copy_assignment = 
+    std::integral_constant<bool, POCCA>;
+  using propagate_on_container_move_assignment = 
+    std::integral_constant<bool, POCMA>;
 
   int x_ = -1;
 
-  using value_type = T;
+  template <class U> struct rebind
+  {
+    typedef poca_allocator<U, POCCA, POCMA> other;
+  };
 
+  poca_allocator() = default;
+  poca_allocator(poca_allocator const&) = default;
+  poca_allocator(poca_allocator &&) = default;
+
+  poca_allocator(int const x) : x_{x} {}
+
+  poca_allocator& operator=(poca_allocator const& rhs)
+  {
+    if (this != &rhs) {
+      super::operator=(rhs);
+      x_ = rhs.x_;
+    }
+    return *this;
+  }
+
+  template <class U> poca_allocator(
+    poca_allocator<U, POCCA, POCMA> const& rhs) :
+    super{rhs}, x_{rhs.x_}
+  {
+  }
+
+  pointer allocate(std::size_t n)
+  {
+    auto p = super::allocate(n + 1);
+    reinterpret_cast<char&>(*p) = static_cast<char>(x_);
+    return p + std::ptrdiff_t(1);
+  }
+
+  void deallocate(pointer p, std::size_t n)
+  {
+    p = p + std::ptrdiff_t(-1);
+    BOOST_TEST_EQ(reinterpret_cast<char&>(*p), static_cast<char>(x_));
+    super::deallocate(p, n + 1); 
+  }
+
+  bool operator==(poca_allocator const& rhs) const { return x_ == rhs.x_; }
+  bool operator!=(poca_allocator const& rhs) const { return x_ != rhs.x_; }
+};
+
+template <class T> 
+struct pocca_allocator: poca_allocator<T, true, false>
+{
   pocca_allocator() = default;
   pocca_allocator(pocca_allocator const&) = default;
   pocca_allocator(pocca_allocator &&) = default;
+  using poca_allocator<T, true, false>::poca_allocator;
 
-  pocca_allocator(int const x) : x_{x} {}
+  pocca_allocator& operator=(pocca_allocator const&) = default;
-
-  pocca_allocator& operator=(pocca_allocator const& rhs)
-  {
-    if (this != &rhs) {
-      x_ = rhs.x_;
-    }
-    return *this;
-  }
-
-  template <class U> pocca_allocator(pocca_allocator<U> const& rhs) : x_{rhs.x_}
-  {
-  }
-
-  T* allocate(std::size_t n)
-  {
-    return static_cast<T*>(::operator new(n * sizeof(T)));
-  }
-
-  void deallocate(T* p, std::size_t) { ::operator delete(p); }
-
-  bool operator==(pocca_allocator const& rhs) const { return x_ == rhs.x_; }
-  bool operator!=(pocca_allocator const& rhs) const { return x_ != rhs.x_; }
 };
 
-template <class T> struct pocma_allocator
+template <class T> 
+struct pocma_allocator: poca_allocator<T, false, true>
 {
-  using propagate_on_container_move_assignment = std::true_type;
-
-  int x_ = -1;
-
-  using value_type = T;
-
   pocma_allocator() = default;
   pocma_allocator(pocma_allocator const&) = default;
   pocma_allocator(pocma_allocator &&) = default;
+  using poca_allocator<T, false, true>::poca_allocator;
 
-  pocma_allocator(int const x) : x_{x} {}
+  pocma_allocator& operator=(pocma_allocator const&) = default;
-
-  pocma_allocator& operator=(pocma_allocator const& rhs)
-  {
-    if (this != &rhs) {
-      x_ = rhs.x_;
-    }
-    return *this;
-  }
-
-  template <class U> pocma_allocator(pocma_allocator<U> const& rhs) : x_{rhs.x_}
-  {
-  }
-
-  T* allocate(std::size_t n)
-  {
-    return static_cast<T*>(::operator new(n * sizeof(T)));
-  }
-
-  void deallocate(T* p, std::size_t) { ::operator delete(p); }
-
-  bool operator==(pocma_allocator const& rhs) const { return x_ == rhs.x_; }
-  bool operator!=(pocma_allocator const& rhs) const { return x_ != rhs.x_; }
 };
 
 namespace {
@@ -433,9 +437,6 @@ namespace {
       std::is_nothrow_move_assignable<
         replace_allocator<X, std::allocator> >::value);
 
-    BOOST_STATIC_ASSERT(
-      std::is_nothrow_move_assignable<pocma_container_type>::value);
-
     BOOST_STATIC_ASSERT(
       !std::is_nothrow_move_assignable<
         replace_allocator<X, stateful_allocator> >::value);