Fix incorrect placement new of allocator type by reifying ad hoc node handle implementations into a base class

include/boost/unordered/detail/foa.hpp

@@ -82,6 +82,7 @@
 namespace boost{
 namespace unordered{
 namespace detail{
+namespace foa{
 
 template <class Iterator,class NodeType>
 struct insert_return_type
@@ -91,7 +92,86 @@ struct insert_return_type
   NodeType node;
 };
 
-namespace foa{
+template <class NodeTypes,class Allocator>
+struct node_handle_base
+{
+  protected:
+    using type_policy=NodeTypes;
+    using element_type=typename type_policy::element_type;
+
+  public:
+    using allocator_type = Allocator;
+
+  private:
+    alignas(element_type) unsigned char x_[sizeof(element_type)];
+    alignas(Allocator) unsigned char a_[sizeof(Allocator)];
+    bool empty_=true;
+
+  protected:
+    element_type& element()noexcept
+    {
+      BOOST_ASSERT(!empty());
+      return *reinterpret_cast<element_type*>(x_);
+    }
+
+    element_type const& element()const noexcept
+    {
+      BOOST_ASSERT(!empty());
+      return *reinterpret_cast<element_type const*>(x_);
+    }
+
+    Allocator& al()noexcept
+    {
+      BOOST_ASSERT(!empty());
+      return *reinterpret_cast<Allocator*>(a_);
+    }
+
+    Allocator const& al()const noexcept
+    {
+      BOOST_ASSERT(!empty());
+      return *reinterpret_cast<Allocator const*>(a_);
+    }
+
+    void emplace(element_type x,Allocator a)
+    {
+      BOOST_ASSERT(empty());
+
+      new(x_)element_type(std::move(x));
+      new(a_)Allocator(a);
+      empty_=false;
+    }
+
+  public:
+    constexpr node_handle_base() noexcept = default;
+
+    node_handle_base(node_handle_base&& nh) noexcept
+    {
+      if (!nh.empty()) {
+        // neither of these move constructors are allowed to throw exceptions
+        // so we can get away with rote placement new
+        //
+        new (a_) Allocator(std::move(nh.al()));
+        new (x_) element_type(std::move(nh.element()));
+        empty_ = false;
+
+        reinterpret_cast<Allocator*>(nh.a_)->~Allocator();
+        nh.empty_ = true;
+      }
+    }
+
+    ~node_handle_base()
+    {
+      if(!empty()){
+        type_policy::destroy(al(),reinterpret_cast<element_type*>(x_));
+        reinterpret_cast<Allocator*>(a_)->~Allocator();
+        empty_=true;
+      }
+    }
+
+    allocator_type get_allocator()const noexcept{return al();}
+    explicit operator bool()const noexcept{ return !empty();}
+    BOOST_ATTRIBUTE_NODISCARD bool empty()const noexcept{return empty_;}
+};
 
 static const std::size_t default_bucket_count = 0;
 
@@ -1509,12 +1589,13 @@ public:
     }
   }
 
-  void extract(const_iterator pos, element_type* p)noexcept
+  element_type extract(const_iterator pos)noexcept
   {
     BOOST_ASSERT(pos!=end());
-    type_policy::construct(al(),p,type_policy::move(*pos.p));
+    element_type x=std::move(*pos.p);
     destroy_element(pos.p);
     recover_slot(pos.pc);
+    return x;
   }
 
   // TODO: should we accept different allocator too?

include/boost/unordered/unordered_node_map.hpp

@@ -126,11 +126,16 @@ namespace boost {
         }
       };
 
-      template <class NodeMapTypes, class Allocator> struct node_map_handle
+      template <class NodeMapTypes, class Allocator>
+      struct node_map_handle
+          : public detail::foa::node_handle_base<NodeMapTypes, Allocator>
       {
       private:
-        using type_policy = NodeMapTypes;
+        using base_type =
-        using element_type = typename type_policy::element_type;
+          detail::foa::node_handle_base<NodeMapTypes, Allocator>;
+
+        using base_type::element;
+        using base_type::type_policy;
 
         template <class Key, class T, class Hash, class Pred, class Alloc>
         friend class boost::unordered::unordered_node_map;
@@ -138,76 +143,25 @@ namespace boost {
       public:
         using key_type = typename NodeMapTypes::key_type;
         using mapped_type = typename NodeMapTypes::mapped_type;
-        using allocator_type = Allocator;
-
-      private:
-        alignas(element_type) unsigned char x[sizeof(element_type)];
-        alignas(Allocator) unsigned char a[sizeof(Allocator)];
-        bool empty_;
-
-        element_type& element() noexcept
-        {
-          BOOST_ASSERT(!empty());
-          return *reinterpret_cast<element_type*>(x);
-        }
-
-        element_type const& element() const noexcept
-        {
-          BOOST_ASSERT(!empty());
-          return *reinterpret_cast<element_type const*>(x);
-        }
-
-        Allocator& al() noexcept
-        {
-          BOOST_ASSERT(!empty());
-          return *reinterpret_cast<Allocator*>(a);
-        }
-
-        Allocator const& al() const noexcept
-        {
-          BOOST_ASSERT(!empty());
-          return *reinterpret_cast<Allocator const*>(a);
-        }
 
       public:
+        using base_type::empty;
+
         constexpr node_map_handle() noexcept = default;
-
+        node_map_handle(node_map_handle&& nh) noexcept = default;
-        node_map_handle(node_map_handle&& nh) noexcept
-        {
-          // neither of these move constructors are allowed to throw exceptions
-          // so we can get away with rote placement new
-          //
-          new (a) Allocator(std::move(nh.al()));
-          new (x) element_type(std::move(nh.element()));
-          empty_ = false;
-
-          reinterpret_cast<Allocator*>(nh.a)->~Allocator();
-          nh.empty_ = true;
-        }
-
-        ~node_map_handle()
-        {
-          if (!empty()) {
-            type_policy::destroy(al(), reinterpret_cast<element_type*>(x));
-            reinterpret_cast<Allocator*>(a)->~Allocator();
-            empty_ = true;
-          }
-        }
 
         key_type& key() const
         {
+          BOOST_ASSERT(!empty());
           return const_cast<key_type&>(type_policy::extract(element()));
         }
 
         mapped_type& mapped() const
         {
+          BOOST_ASSERT(!empty());
           return const_cast<mapped_type&>(
             type_policy::value_from(element()).second);
         }
-
-        allocator_type get_allocator() const noexcept { return al(); }
-        explicit operator bool() const noexcept { return !empty(); }
-        BOOST_ATTRIBUTE_NODISCARD bool empty() const noexcept { return empty_; }
       };
     } // namespace detail
 
@@ -247,7 +201,7 @@ namespace boost {
         typename boost::allocator_rebind<Allocator,
           typename map_types::value_type>::type>;
       using insert_return_type =
-        detail::insert_return_type<iterator, node_type>;
+        detail::foa::insert_return_type<iterator, node_type>;
 
       unordered_node_map() : unordered_node_map(0) {}
 
@@ -625,10 +579,8 @@ namespace boost {
       {
         BOOST_ASSERT(pos != end());
         node_type nh;
-        table_.extract(
+        auto elem = table_.extract(pos);
-          pos, reinterpret_cast<typename map_types::element_type*>(nh.x));
+        nh.emplace(std::move(elem), get_allocator());
-        new (&nh.a) allocator_type(get_allocator());
-        nh.empty_ = false;
         return nh;
       }
 
@@ -637,12 +589,8 @@ namespace boost {
         auto pos = find(key);
         node_type nh;
         if (pos != end()) {
-          table_.extract(
+          auto elem = table_.extract(pos);
-            pos, reinterpret_cast<typename map_types::element_type*>(nh.x));
+          nh.emplace(std::move(elem), get_allocator());
-          new (&nh.a) allocator_type(get_allocator());
-          nh.empty_ = false;
-        } else {
-          nh.empty_ = true;
         }
         return nh;
       }
@@ -657,12 +605,8 @@ namespace boost {
         auto pos = find(key);
         node_type nh;
         if (pos != end()) {
-          table_.extract(
+          auto elem = table_.extract(pos);
-            pos, reinterpret_cast<typename map_types::element_type*>(nh.x));
+          nh.emplace(std::move(elem), get_allocator());
-          new (&nh.a) allocator_type(get_allocator());
-          nh.empty_ = false;
-        } else {
-          nh.empty_ = true;
         }
         return nh;
       }

include/boost/unordered/unordered_node_set.hpp

@@ -108,82 +108,34 @@ namespace boost {
         }
       };
 
-      template <class NodeSetTypes, class Allocator> struct node_set_handle
+      template <class NodeSetTypes, class Allocator>
+      struct node_set_handle
+          : public detail::foa::node_handle_base<NodeSetTypes, Allocator>
       {
       private:
-        using type_policy = NodeSetTypes;
+        using base_type =
-        using element_type = typename type_policy::element_type;
+          detail::foa::node_handle_base<NodeSetTypes, Allocator>;
+
+        using base_type::element;
+        using base_type::type_policy;
 
         template <class Key, class Hash, class Pred, class Alloc>
         friend class boost::unordered::unordered_node_set;
 
       public:
         using value_type = typename NodeSetTypes::value_type;
-        using allocator_type = Allocator;
-
-      private:
-        alignas(element_type) unsigned char x[sizeof(element_type)];
-        alignas(Allocator) unsigned char a[sizeof(Allocator)];
-        bool empty_;
-
-        element_type& element() noexcept
-        {
-          BOOST_ASSERT(!empty());
-          return *reinterpret_cast<element_type*>(x);
-        }
-
-        element_type const& element() const noexcept
-        {
-          BOOST_ASSERT(!empty());
-          return *reinterpret_cast<element_type const*>(x);
-        }
-
-        Allocator& al() noexcept
-        {
-          BOOST_ASSERT(!empty());
-          return *reinterpret_cast<Allocator*>(a);
-        }
-
-        Allocator const& al() const noexcept
-        {
-          BOOST_ASSERT(!empty());
-          return *reinterpret_cast<Allocator const*>(a);
-        }
 
       public:
+        using base_type::empty;
+
         constexpr node_set_handle() noexcept = default;
-
+        node_set_handle(node_set_handle&& nh) noexcept = default;
-        node_set_handle(node_set_handle&& nh) noexcept
-        {
-          // neither of these move constructors are allowed to throw exceptions
-          // so we can get away with rote placement new
-          //
-          new (a) Allocator(std::move(nh.al()));
-          new (x) element_type(std::move(nh.element()));
-          empty_ = false;
-
-          reinterpret_cast<Allocator*>(nh.a)->~Allocator();
-          nh.empty_ = true;
-        }
-
-        ~node_set_handle()
-        {
-          if (!empty()) {
-            type_policy::destroy(al(), reinterpret_cast<element_type*>(x));
-            reinterpret_cast<Allocator*>(a)->~Allocator();
-            empty_ = true;
-          }
-        }
 
         value_type& value() const
         {
           BOOST_ASSERT(!empty());
           return const_cast<value_type&>(type_policy::extract(element()));
         }
-
-        allocator_type get_allocator() const noexcept { return al(); }
-        explicit operator bool() const noexcept { return !empty(); }
-        BOOST_ATTRIBUTE_NODISCARD bool empty() const noexcept { return empty_; }
       };
     } // namespace detail
 
@@ -222,7 +174,7 @@ namespace boost {
         typename boost::allocator_rebind<Allocator,
           typename set_types::value_type>::type>;
       using insert_return_type =
-        detail::insert_return_type<iterator, node_type>;
+        detail::foa::insert_return_type<iterator, node_type>;
 
       unordered_node_set() : unordered_node_set(0) {}
 
@@ -503,10 +455,8 @@ namespace boost {
       {
         BOOST_ASSERT(pos != end());
         node_type nh;
-        table_.extract(
+        auto elem = table_.extract(pos);
-          pos, reinterpret_cast<typename set_types::element_type*>(nh.x));
+        nh.emplace(std::move(elem), get_allocator());
-        new (&nh.a) allocator_type(get_allocator());
-        nh.empty_ = false;
         return nh;
       }
 
@@ -515,12 +465,8 @@ namespace boost {
         auto pos = find(key);
         node_type nh;
         if (pos != end()) {
-          table_.extract(
+          auto elem = table_.extract(pos);
-            pos, reinterpret_cast<typename set_types::element_type*>(nh.x));
+          nh.emplace(std::move(elem), get_allocator());
-          new (&nh.a) allocator_type(get_allocator());
-          nh.empty_ = false;
-        } else {
-          nh.empty_ = true;
         }
         return nh;
       }
@@ -535,12 +481,8 @@ namespace boost {
         auto pos = find(key);
         node_type nh;
         if (pos != end()) {
-          table_.extract(
+          auto elem = table_.extract(pos);
-            pos, reinterpret_cast<typename set_types::element_type*>(nh.x));
+          nh.emplace(std::move(elem), get_allocator());
-          new (&nh.a) allocator_type(get_allocator());
-          nh.empty_ = false;
-        } else {
-          nh.empty_ = true;
         }
         return nh;
       }