Problems with delete in destructor

Question

Hello, 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.

Answer 3

Use global function operator new instead of operator new. It will allocate memory but will not call constructors. Same for delete:

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 = static_cast<T*>(::operator new[] (sizeof(T)*capacity)); //memory allocating 
   for(T* p = mem_ptr; p < mem_ptr + capacity; p ++ ) { 
       ::new (p) T (default_value); //initialization 
   } 
} 
~CyclicalArray() { 
   for(T* p = mem_ptr; p < mem_ptr + capacity; p ++) { 
      p->~T();
   } 
   ::operator delete[]( static_cast<void*>(mem_ptr) ); 
}

Answer 4

You are calling your constructors and destructors twice because you are using a new-expression and a delete-expression:

// This allocates and calls constructor
mem_ptr = ::new T[size];

// This calls the destructor and deallocates the memory
delete[] mem_ptr;

If you want to just allocate raw memory, you can explicitly call operator new:

// This simply allocates raw memory
mem_ptr = ::operator new(sizeof(T) * size);

// And this simply deallocates memory
::operator delete(mem_ptr);

Answer 5

Why use placement new at all? That code basically boils down to this:

template <class T>
class CyclicalArray { 
private: 
    T* mem_ptr;
    size_t capacity;
    T default_value;
    size_t head_index;
public: 
    CyclicalArray(size_t capacity, const T& default_value) : capacity(capacity), default_value(default_value), head_index(0) {
       mem_ptr = new T[capacity](default_value); //memory allocating and construction
    }

    ~CyclicalArray() { 
        delete[] mem_ptr; 
    }
};

EDIT: If you do want to use placement new, your loop should look like this:

for(T* p = mem_ptr; p != mem_ptr + capacity; ++p) { 

No need to scale things by sizeof(T), that is done for you in C/C++.