Problems with delete in destructor

Question

I wrote this code.
The constructor works normally, but in the destructor I get "Windows has triggered a breakpoint." How should I correct this?

template class CyclicalArray { 
private: 
   T* mem_ptr;
public: 
   CyclicalArray(size_t capacity, const T& default_value) {
   this->default_value = default_value; 
   this->capacity = capacity; 
   head_index = 0; 
   mem_ptr = ::new T[capacity]; //memory allocating 
   for(T* p = mem_ptr; p < mem_ptr + capacity * sizeof(T); p += sizeof(T)) { 
       ::new (p) T (default_value); //initialization 
   } 
} 
~CyclicalArray() { 
   for(T* p = mem_ptr + sizeof(T); p < mem_ptr + capacity * sizeof(T); p += sizeof(T)) { 
      p->~T();
   } 
   delete[] mem_ptr; 
}

Answer 1

If you're going to perform placement new, you need to do it on raw memory. Something like:

template class CyclicalArray { 
private: 
   T* mem_ptr;
public: 
   CyclicalArray(size_t capacity, const T& default_value) {
   this->default_value = default_value; 
   this->capacity = capacity; 
   head_index = 0; 
   mem_ptr = reinterpret_cast<T*>( ::new char[capacity * sizeof(T)]); //memory allocating 
   for(T* p = mem_ptr; p < mem_ptr + capacity; ++p) { 
       ::new (p) T (default_value); //initialization 
   } 
} 
~CyclicalArray() { 
   // this 
   for(T* p = mem_ptr + capacity; p != mem_ptr; --p) { 
      (p-1)->~T();
   } 
   delete[] reinterpret_cast<char*>( mem_ptr); 
}

Otherwise you'll call the T destructor twice on the same object memory (not a good thing to do).

Also, since your p pointers are of type T*, you can perform simple increment/decrements on it - the compiler will deal with the sizeof(T) issue as a normal course of pointer arithmetic.

Finally, strictly speaking you should destroy the array elements in descending order (the opposite of construction).

I hope this catches most or all the bugs.

You might really want to consider using something like std::vector as the store. An example using std::vector<> follows (with a few other syntax fixes). I'm not sure if your class would really need a copy of the default_value or the head_index - I left them in assuming that you plan to use them in other methods:

#include <vector>

template <typename T>
class CyclicalArray { 
private: 
   std::vector<T> backing_store;
   T default_value;
   size_t head_index;

public: 
    CyclicalArray(size_t capacity, const T& def_val) : 
        backing_store(capacity, def_val), 
        default_value( def_val), 
        head_index(0) {
    } 

    ~CyclicalArray() {}
};

Note how much simpler the constructor and destructor are, since all the complexity of your first class is managed by std:vector.

Answer 2

You're probably going way beyond the end of the mem_ptr array. In C and C++, pointer arithmetic is in units of the type involved, not bytes. For example, if you have int *a;, then if a is 0x100, and sizeof(int) == 4, a + 1 is 0x104.

Therefore, you're incrementing p by the size of the type squared, since adding 1 to it will move it sizeof(T) bytes, and so adding sizeof(T) to it will increment it far too much.

Not to mention that you don't need to call individual destructors in an array, since delete [] takes care of that for you.