Add move-semantic to Compact_container

That required a refactoring the timestamper feature. And the
explanation why is quite long... Let's do it.

The CGAL triangulations use the `Rebind_TDS` feature. During the
instanciation of `Compact_container<T>` it is important that `T` is
allowed to be incomplete, otherwise the circular dependencies between
TDS and Vertex/Cell cannot be resolved by the compiler. In previous
implementation of the timestamper, to allow `T` to be an incomplete
type, the compact container only carried a *point* to the time
stamper, allocated on the heap. A moved-from compact container would
only be valid if one recreated a time stamper for it on the head in
the move-constructor. As I want move operations to be `noexcept`, I
needed to change that implementation. Now the triangulation always
carries a time stamp counter (`std::size`), independently of the type
`T`, and the time stamper is an empty class, with static methods. That
allows `T` to be incomplete during the declaration of
`Compact_container<T>`.

STL_Extension/include/CGAL/Compact_container.h

@@ -25,6 +25,7 @@
 #include <vector>
 #include <cstring>
 #include <functional>
+#include <atomic>
 
 #include <CGAL/memory.h>
 #include <CGAL/iterator.h>
@@ -223,7 +224,8 @@ class Compact_container
 public:
   typedef typename Default::Get< TimeStamper_,
                                  CGAL::Time_stamper_impl<T> >::type
-                                                    Time_stamper_impl;
+                                                    Time_stamper;
+  typedef Time_stamper                              Time_stamper_impl; // backward-compatibility
 
   typedef T                                         value_type;
   typedef Allocator                                 allocator_type;
@@ -250,16 +252,14 @@ public:
 
   explicit Compact_container(const Allocator &a = Allocator())
   : alloc(a)
-  , time_stamper(new Time_stamper_impl())
   {
-    init ();
+    init();
   }
 
   template < class InputIterator >
   Compact_container(InputIterator first, InputIterator last,
                     const Allocator & a = Allocator())
   : alloc(a)
-  , time_stamper(new Time_stamper_impl())
   {
     init();
     std::copy(first, last, CGAL::inserter(*this));
@@ -268,14 +268,19 @@ public:
   // The copy constructor and assignment operator preserve the iterator order
   Compact_container(const Compact_container &c)
   : alloc(c.get_allocator())
-  , time_stamper(new Time_stamper_impl())
   {
     init();
     block_size = c.block_size;
-    *time_stamper = *c.time_stamper;
+    time_stamp = c.time_stamp.load();
     std::copy(c.begin(), c.end(), CGAL::inserter(*this));
   }
 
+  Compact_container(Compact_container&& c) noexcept
+  : alloc(c.get_allocator())
+  {
+    c.swap(*this);
+  }
+
   Compact_container & operator=(const Compact_container &c)
   {
     if (&c != this) {
@@ -285,10 +290,16 @@ public:
     return *this;
   }
 
+  Compact_container & operator=(Compact_container&& c) noexcept
+  {
+    Self tmp(std::move(c));
+    tmp.swap(*this);
+    return *this;
+  }
+
   ~Compact_container()
   {
     clear();
-    delete time_stamper;
   }
 
   bool is_used(const_iterator ptr) const
@@ -328,17 +339,8 @@ public:
     return all_items[block_number].first[index_in_block];
   }
 
-  void swap(Self &c)
+  friend void swap(Compact_container& a, Compact_container b) {
-  {
+    a.swap(b);
-    std::swap(alloc, c.alloc);
-    std::swap(capacity_, c.capacity_);
-    std::swap(size_, c.size_);
-    std::swap(block_size, c.block_size);
-    std::swap(first_item, c.first_item);
-    std::swap(last_item, c.last_item);
-    std::swap(free_list, c.free_list);
-    all_items.swap(c.all_items);
-    std::swap(time_stamper, c.time_stamper);
   }
 
   iterator begin() { return iterator(first_item, 0, 0); }
@@ -383,7 +385,7 @@ public:
     new (ret) value_type(args...);
     CGAL_assertion(type(ret) == USED);
     ++size_;
-    time_stamper->set_time_stamp(ret);
+    Time_stamper::set_time_stamp(ret, time_stamp);
     return iterator(ret, 0);
   }
 
@@ -397,7 +399,7 @@ public:
     std::allocator_traits<allocator_type>::construct(alloc, ret, t);
     CGAL_assertion(type(ret) == USED);
     ++size_;
-    time_stamper->set_time_stamp(ret);
+    Time_stamper::set_time_stamp(ret, time_stamp);
     return iterator(ret, 0);
   }
 
@@ -652,7 +654,21 @@ public:
   static bool is_begin_or_end(const_pointer ptr)
   { return type(ptr)==START_END; }
 
+  void swap(Self &c)
+  {
+    std::swap(alloc, c.alloc);
+    std::swap(capacity_, c.capacity_);
+    std::swap(size_, c.size_);
+    std::swap(block_size, c.block_size);
+    std::swap(first_item, c.first_item);
+    std::swap(last_item, c.last_item);
+    std::swap(free_list, c.free_list);
+    all_items.swap(c.all_items);
 
+    // non-atomic swap of time_stamp:
+    c.time_stamp = time_stamp.exchange(c.time_stamp.load());
+  }
+private:
   // We store a vector of pointers to all allocated blocks and their sizes.
   // Knowing all pointers, we don't have to walk to the end of a block to reach
   // the pointer to the next block.
@@ -660,7 +676,9 @@ public:
   // by walking through the block till its end.
   // This opens up the possibility for the compiler to optimize the clear()
   // function considerably when has_trivial_destructor<T>.
-  typedef std::vector<std::pair<pointer, size_type> >  All_items;
+  using All_items = std::vector<std::pair<pointer, size_type> >;
+
+  using time_stamp_t = std::atomic<std::size_t>;
 
   void init()
   {
@@ -671,21 +689,18 @@ public:
     first_item = nullptr;
     last_item  = nullptr;
     all_items  = All_items();
-    time_stamper->reset();
+    time_stamp = 0;
   }
 
   allocator_type   alloc;
-  size_type        capacity_;
+  size_type        capacity_   = 0;
-  size_type        size_;
+  size_type        size_       = 0;
-  size_type        block_size;
+  size_type        block_size  = Increment_policy::first_block_size;
-  pointer          free_list;
+  pointer          free_list   = nullptr;
-  pointer          first_item;
+  pointer          first_item  = nullptr;
-  pointer          last_item;
+  pointer          last_item   = nullptr;
-  All_items        all_items;
+  All_items        all_items   = {};
-
+  time_stamp_t     time_stamp  = {};
-  // This is a pointer, so that the definition of Compact_container does
-  // not require a complete type `T`.
-  Time_stamper_impl* time_stamper;
 };
 
 template < class T, class Allocator, class Increment_policy, class TimeStamper >
@@ -759,7 +774,7 @@ void Compact_container<T, Allocator, Increment_policy, TimeStamper>::allocate_ne
   for (size_type i = block_size; i >= 1; --i)
   {
     EraseCounterStrategy::set_erase_counter(*(new_block + i), 0);
-    time_stamper->initialize_time_stamp(new_block + i);
+    Time_stamper::initialize_time_stamp(new_block + i);
     put_on_free_list(new_block + i);
   }
   // We insert this new block at the end.
@@ -839,7 +854,6 @@ namespace internal {
   template < class DSC, bool Const >
   class CC_iterator
   {
-    typedef typename DSC::iterator                    iterator;
     typedef CC_iterator<DSC, Const>                   Self;
   public:
     typedef DSC                                       CC;
@@ -861,26 +875,52 @@ namespace internal {
       m_ptr.p = nullptr;
     }
 
-    // Either a harmless copy-ctor,
+    CC_iterator (const CC_iterator &it)
-    // or a conversion from iterator to const_iterator.
-    CC_iterator (const iterator &it)
 #ifdef CGAL_COMPACT_CONTAINER_DEBUG_TIME_STAMP
-      : ts(Time_stamper_impl::time_stamp(it.operator->()))
+      : ts(Time_stamper::time_stamp(it.operator->()))
 #endif
     {
       m_ptr.p = it.operator->();
     }
 
-    // Same for assignment operator (otherwise MipsPro warns)
+    // Converting constructor from mutable to constant iterator
-    CC_iterator & operator= (const iterator &it)
+    template <bool OtherConst>
-    {
+    CC_iterator(const CC_iterator<
-      m_ptr.p = it.operator->();
+                typename std::enable_if<(Const && !OtherConst), DSC>::type,
+                OtherConst> &const_it)
 #ifdef CGAL_COMPACT_CONTAINER_DEBUG_TIME_STAMP
-      ts = Time_stamper_impl::time_stamp(it.operator->());
+        : ts(Time_stamper::time_stamp(const_it.operator->()))
+#endif
+    {
+      m_ptr.p = const_it.operator->();
+    }
+
+    // Assignment operator from mutable to constant iterator
+    template <bool OtherConst>
+    CC_iterator & operator= (const CC_iterator<
+                typename std::enable_if<(Const && !OtherConst), DSC>::type,
+                OtherConst> &const_it)
+    {
+      m_ptr.p = const_it.operator->();
+#ifdef CGAL_COMPACT_CONTAINER_DEBUG_TIME_STAMP
+      ts = Time_stamper::time_stamp(const_it.operator->());
 #endif
       return *this;
     }
 
+    CC_iterator(CC_iterator&& it) noexcept
+#ifdef CGAL_COMPACT_CONTAINER_DEBUG_TIME_STAMP
+      : ts(Time_stamper::time_stamp(it.operator->()))
+#endif
+    {
+      m_ptr.p = it.operator->();
+      it.m_ptr.p = nullptr;
+    }
+
+    ~CC_iterator() = default;
+    CC_iterator& operator=(const CC_iterator&) = default;
+    CC_iterator& operator=(CC_iterator&&) = default;
+
     // Construction from nullptr
     CC_iterator (std::nullptr_t CGAL_assertion_code(n))
 #ifdef CGAL_COMPACT_CONTAINER_DEBUG_TIME_STAMP
@@ -893,7 +933,7 @@ namespace internal {
 
   private:
 
-    typedef typename DSC::Time_stamper_impl           Time_stamper_impl;
+    typedef typename DSC::Time_stamper           Time_stamper;
 #ifdef CGAL_COMPACT_CONTAINER_DEBUG_TIME_STAMP
     std::size_t ts;
 #endif
@@ -925,7 +965,7 @@ namespace internal {
         increment();
 #ifdef CGAL_COMPACT_CONTAINER_DEBUG_TIME_STAMP
       else
-        ts = Time_stamper_impl::time_stamp(m_ptr.p);
+        ts = Time_stamper::time_stamp(m_ptr.p);
 #endif // CGAL_COMPACT_CONTAINER_DEBUG_TIME_STAMP
     }
 
@@ -938,7 +978,7 @@ namespace internal {
       m_ptr.p = ptr;
 #ifdef CGAL_COMPACT_CONTAINER_DEBUG_TIME_STAMP
       if(ptr != nullptr){
-        ts = Time_stamper_impl::time_stamp(m_ptr.p);
+        ts = Time_stamper::time_stamp(m_ptr.p);
       }
 #endif // end CGAL_COMPACT_CONTAINER_DEBUG_TIME_STAMP
     }
@@ -959,7 +999,7 @@ namespace internal {
             DSC::type(m_ptr.p) == DSC::START_END)
         {
 #ifdef CGAL_COMPACT_CONTAINER_DEBUG_TIME_STAMP
-          ts = Time_stamper_impl::time_stamp(m_ptr.p);
+          ts = Time_stamper::time_stamp(m_ptr.p);
 #endif
           return;
         }
@@ -983,7 +1023,7 @@ namespace internal {
             DSC::type(m_ptr.p) == DSC::START_END)
         {
 #ifdef CGAL_COMPACT_CONTAINER_DEBUG_TIME_STAMP
-          ts = Time_stamper_impl::time_stamp(m_ptr.p);
+          ts = Time_stamper::time_stamp(m_ptr.p);
 #endif
           return;
         }
@@ -1022,7 +1062,7 @@ namespace internal {
 #ifdef CGAL_COMPACT_CONTAINER_DEBUG_TIME_STAMP
     bool is_time_stamp_valid() const
     {
-      return (ts == 0) || (ts == Time_stamper_impl::time_stamp(m_ptr.p));
+      return (ts == 0) || (ts == Time_stamper::time_stamp(m_ptr.p));
     }
 #endif // CGAL_COMPACT_CONTAINER_DEBUG_TIME_STAMP
 
@@ -1036,7 +1076,7 @@ namespace internal {
 #ifdef CGAL_COMPACT_CONTAINER_DEBUG_TIME_STAMP
       assert( is_time_stamp_valid() );
 #endif
-      return Time_stamper_impl::less(m_ptr.p, other.m_ptr.p);
+      return Time_stamper::less(m_ptr.p, other.m_ptr.p);
     }
 
     bool operator>(const CC_iterator& other) const
@@ -1044,7 +1084,7 @@ namespace internal {
 #ifdef CGAL_COMPACT_CONTAINER_DEBUG_TIME_STAMP
       assert( is_time_stamp_valid() );
 #endif
-      return Time_stamper_impl::less(other.m_ptr.p, m_ptr.p);
+      return Time_stamper::less(other.m_ptr.p, m_ptr.p);
     }
 
     bool operator<=(const CC_iterator& other) const
@@ -1052,7 +1092,7 @@ namespace internal {
 #ifdef CGAL_COMPACT_CONTAINER_DEBUG_TIME_STAMP
       assert( is_time_stamp_valid() );
 #endif
-      return Time_stamper_impl::less(m_ptr.p, other.m_ptr.p)
+      return Time_stamper::less(m_ptr.p, other.m_ptr.p)
           || (*this == other);
     }
 
@@ -1061,7 +1101,7 @@ namespace internal {
 #ifdef CGAL_COMPACT_CONTAINER_DEBUG_TIME_STAMP
       assert( is_time_stamp_valid() );
 #endif
-      return Time_stamper_impl::less(other.m_ptr.p, m_ptr.p)
+      return Time_stamper::less(other.m_ptr.p, m_ptr.p)
           || (*this == other);
     }
 

STL_Extension/include/CGAL/Concurrent_compact_container.h

@@ -210,7 +210,8 @@ class Concurrent_compact_container
   typedef Concurrent_compact_container_traits <T>                   Traits;
 
 public:
-  typedef CGAL::Time_stamper_impl<T>                Time_stamper_impl;
+  typedef CGAL::Time_stamper_impl<T>                Time_stamper;
+  typedef Time_stamper                              Time_stamper_impl; // backward compatibility
 
   typedef T                                         value_type;
   typedef Allocator                                 allocator_type;
@@ -241,7 +242,6 @@ public:
 
   explicit Concurrent_compact_container(const Allocator &a = Allocator())
   : m_alloc(a)
-  , m_time_stamper(new Time_stamper_impl())
   {
     init ();
   }
@@ -250,7 +250,6 @@ public:
   Concurrent_compact_container(InputIterator first, InputIterator last,
                     const Allocator & a = Allocator())
   : m_alloc(a)
-  , m_time_stamper(new Time_stamper_impl())
   {
     init();
     std::copy(first, last, CGAL::inserter(*this));
@@ -259,13 +258,18 @@ public:
   // The copy constructor and assignment operator preserve the iterator order
   Concurrent_compact_container(const Concurrent_compact_container &c)
   : m_alloc(c.get_allocator())
-  , m_time_stamper(new Time_stamper_impl())
   {
     init();
     m_block_size = c.m_block_size;
     std::copy(c.begin(), c.end(), CGAL::inserter(*this));
   }
 
+  Concurrent_compact_container(Concurrent_compact_container&& c) noexcept
+  : m_alloc(c.get_allocator())
+  {
+    c.swap(*this);
+  }
+
   Concurrent_compact_container & operator=(const Concurrent_compact_container &c)
   {
     if (&c != this) {
@@ -275,10 +279,16 @@ public:
     return *this;
   }
 
+  Concurrent_compact_container & operator=(Concurrent_compact_container&& c) noexcept
+  {
+    Self tmp(std::move(c));
+    tmp.swap(*this);
+    return *this;
+  }
+
   ~Concurrent_compact_container()
   {
     clear();
-    delete m_time_stamper;
   }
 
   bool is_used(const_iterator ptr) const
@@ -290,11 +300,8 @@ public:
   {
     std::swap(m_alloc, c.m_alloc);
 #if CGAL_CONCURRENT_COMPACT_CONTAINER_APPROXIMATE_SIZE
-    { // non-atomic swap
+    // non-atomic swap of m_capacity
-      size_type other_capacity = c.m_capacity;
+    c.m_capacity = m_capacity.exchange(c.m_capacity.load());
-      c.m_capacity = size_type(m_capacity);
-      m_capacity = other_capacity;
-    }
 #else // not CGAL_CONCURRENT_COMPACT_CONTAINER_APPROXIMATE_SIZE
     std::swap(m_capacity, c.m_capacity);
 #endif // not CGAL_CONCURRENT_COMPACT_CONTAINER_APPROXIMATE_SIZE
@@ -304,7 +311,12 @@ public:
     std::swap(m_last_item, c.m_last_item);
     std::swap(m_free_lists, c.m_free_lists);
     m_all_items.swap(c.m_all_items);
-    std::swap(m_time_stamper, c.m_time_stamper);
+    // non-atomic swap of m_time_stamp
+    c.m_time_stamp = m_time_stamp.exchange(c.m_time_stamp.load());
+  }
+
+  friend void swap(Concurrent_compact_container& a, Concurrent_compact_container& b) {
+    a.swap(b);
   }
 
   iterator begin() { return iterator(m_first_item, 0, 0); }
@@ -544,7 +556,7 @@ private:
   {
     CGAL_assertion(type(ret) == USED);
     fl->dec_size();
-    m_time_stamper->set_time_stamp(ret);
+    Time_stamper::set_time_stamp(ret, m_time_stamp);
     return iterator(ret, 0);
   }
 
@@ -619,8 +631,9 @@ private:
   // by walking through the block till its end.
   // This opens up the possibility for the compiler to optimize the clear()
   // function considerably when has_trivial_destructor<T>.
-  typedef std::vector<std::pair<pointer, size_type> >  All_items;
+  using All_items = std::vector<std::pair<pointer, size_type> >;
 
+  using time_stamp_t = std::atomic<std::size_t>;
 
   void init()
   {
@@ -636,25 +649,23 @@ private:
     m_first_item = nullptr;
     m_last_item  = nullptr;
     m_all_items  = All_items();
-    m_time_stamper->reset();
+    m_time_stamp = 0;
   }
 
   allocator_type    m_alloc;
 #if CGAL_CONCURRENT_COMPACT_CONTAINER_APPROXIMATE_SIZE
-  std::atomic<size_type> m_capacity;
+  std::atomic<size_type> m_capacity = {};
 #else // not CGAL_CONCURRENT_COMPACT_CONTAINER_APPROXIMATE_SIZE
-  size_type         m_capacity;
+  size_type         m_capacity      = {};
 #endif // not CGAL_CONCURRENT_COMPACT_CONTAINER_APPROXIMATE_SIZE
-  size_type         m_block_size;
+  size_type         m_block_size    = CGAL_INIT_CONCURRENT_COMPACT_CONTAINER_BLOCK_SIZE;
   Free_lists        m_free_lists;
-  pointer           m_first_item;
+  pointer           m_first_item    = nullptr;
-  pointer           m_last_item;
+  pointer           m_last_item     = nullptr;
-  All_items         m_all_items;
+  All_items         m_all_items     = {};
   mutable Mutex     m_mutex;
+  time_stamp_t      m_time_stamp    = {};
 
-  // This is a pointer, so that the definition of Compact_container does
-  // not require a complete type `T`.
-  Time_stamper_impl* m_time_stamper;
 };
 
 template < class T, class Allocator >
@@ -770,7 +781,7 @@ void Concurrent_compact_container<T, Allocator>::
   for (size_type i = old_block_size; i >= 1; --i)
   {
     EraseCounterStrategy::set_erase_counter(*(new_block + i), 0);
-    m_time_stamper->initialize_time_stamp(new_block + i);
+    Time_stamper::initialize_time_stamp(new_block + i);
     put_on_free_list(new_block + i, fl);
   }
 }

STL_Extension/include/CGAL/Time_stamper.h

@@ -28,26 +28,12 @@ constexpr size_t rounded_down_log2(size_t n)
 template <typename T>
 struct Time_stamper
 {
-  Time_stamper()
-   : time_stamp_() {}
-
-  Time_stamper(const Time_stamper& ts)
-    : time_stamp_()
-  {
-    time_stamp_ = std::size_t(ts.time_stamp_);
-  }
-
-  Time_stamper& operator=(const Time_stamper& ts)
-  {
-    time_stamp_ = std::size_t(ts.time_stamp_);
-    return *this;
-  }
-
   static void initialize_time_stamp(T* pt) {
     pt->set_time_stamp(std::size_t(-1));
   }
 
-  void set_time_stamp(T* pt) {
+  template <typename time_stamp_t>
+  static void set_time_stamp(T* pt, time_stamp_t& time_stamp_) {
     if(pt->time_stamp() == std::size_t(-1)) {
       const std::size_t new_ts = time_stamp_++;
       pt->set_time_stamp(new_ts);
@@ -96,23 +82,14 @@ struct Time_stamper
       return time_stamp(p_t1) < time_stamp(p_t2);
     }
   }
-
-  void reset() {
-    time_stamp_ = 0;
-  }
-private:
-#ifdef CGAL_NO_ATOMIC
-  std::size_t time_stamp_;
-#else
-  CGAL::cpp11::atomic<std::size_t> time_stamp_;
-#endif
 }; // end class template Time_stamper<T>
 
 template <typename T>
 struct No_time_stamp
 {
 public:
-  void set_time_stamp(T*)  {}
+  template <typename time_stamp_t>
+  static void set_time_stamp(T*, time_stamp_t&)  {}
   static bool less(const T* p_t1,const T* p_t2) {
     return p_t1 < p_t2;
   }
@@ -130,8 +107,6 @@ public:
     constexpr std::size_t shift = internal::rounded_down_log2(sizeof(T));
     return reinterpret_cast<std::size_t>(p) >> shift;
   }
-
-  void reset()                {}
 }; // end class template No_time_stamp<T>
 
 // That class template is an auxiliary class.  It has a

STL_Extension/test/STL_Extension/test_Compact_container.cpp

@@ -4,6 +4,7 @@
 #include <cstddef>
 #include <list>
 #include <vector>
+#include <type_traits>
 #include <CGAL/Compact_container.h>
 #include <CGAL/Random.h>
 
@@ -83,6 +84,11 @@ inline bool check_empty(const Cont &c)
 template < class Cont >
 void test(const Cont &)
 {
+  static_assert(std::is_nothrow_move_constructible<Cont>::value,
+                "move cstr is missing");
+  static_assert(std::is_nothrow_move_assignable<Cont>::value,
+                "move assignment is missing");
+
   // Testing if all types are provided.
 
   typename Cont::value_type              t0;
@@ -319,22 +325,22 @@ int main()
 
   // Check the time stamper policies
   if(! boost::is_base_of<CGAL::Time_stamper<T1>,
-     C1::Time_stamper_impl>::value)
+     C1::Time_stamper>::value)
   {
     std::cerr << "Error timestamper of C1\n"; return 1;
   }
   if(! boost::is_base_of<CGAL::No_time_stamp<T2>,
-     C2::Time_stamper_impl>::value)
+     C2::Time_stamper>::value)
   {
     std::cerr << "Error timestamper of C2\n"; return 1;
   }
   if(! boost::is_base_of<CGAL::No_time_stamp<T3>,
-     C3::Time_stamper_impl>::value)
+     C3::Time_stamper>::value)
   {
     std::cerr << "Error timestamper of C3\n"; return 1;
   }
   if(! boost::is_base_of<CGAL::Time_stamper<T2>,
-     C4::Time_stamper_impl>::value)
+     C4::Time_stamper>::value)
   {
     std::cerr << "Error timestamper of C4\n"; return 1;
   }

STL_Extension/test/STL_Extension/test_Concurrent_compact_container.cpp

@@ -183,6 +183,10 @@ private:
 template < class Cont >
 void test(const Cont &)
 {
+  static_assert(std::is_nothrow_move_constructible<Cont>::value,
+                "move cstr is missing");
+  static_assert(std::is_nothrow_move_assignable<Cont>::value,
+                "move assignment is missing");
   // Testing if all types are provided.
 
   typename Cont::value_type              t0;