Deleting a member array of pointers, then reallocating it

Question

I'm creating a stack class as an exercise trying to learn some c++ concepts (initializer lists, memory management and templates here). I've run into something that I can't get my head around.

In function void Stack::push(const T& item), if I uncomment the delete [] data; line, my code runs well when the template argument is for example int or char. But with std::string, I get weird memory errors.

My thinking here is that I need a bigger array -> arrays can't be resized -> I create a new one -> I deallocate the memory I needed for the one that's soon to be not needed -> I make the existing pointer point to a new memory address where I create the bigger array.

Now, when I comment the delete line, the code runs well even with std::string, but I can't see why I can't do the delete operation safely with all types.

Any insights will be appreciated.

#include <iostream>
#include <stdio.h>
#include <memory.h>

template<class T>
class Stack
{
    T* data;
    int sz;

public:
    //Stack(){sz=0;}
    Stack(const std::initializer_list<T>&);
    ~Stack();

    void push(const T&);
    T& pop();

    void show() const;
};

template<class T>
Stack<T>::Stack(const std::initializer_list<T> &list)
{
    sz=0;
    data = new T[list.size()];

    for (auto i : list) {
        data[sz] = i;
        ++sz;
    }
    std::cout<< "Created with sz:  "<< sz<<std::endl;
}

template<class T>
Stack<T>::~Stack()
{
    delete [] data;
}

template<class T> 
void Stack<T>::push(const T& item) {
    std::cout<<"push "<<item<<std::endl;
    T* arr = new T[sz];
    memcpy(arr, data, sz*sizeof(T));
    //delete [] data;
    data = new T[sz + 1];
    memcpy(data, arr, sz*sizeof(T));
    ++sz;
    data[sz - 1] = item;
    std::cout<<"new size: "<<sz<<", bytes: "<<sz*sizeof(T)<<std::endl;
}

template<class T>
T& Stack<T>::pop()
{
    if(sz > 0) {
        std::cout<<"pop "<<data[sz-1]<<std::endl;
        std::cout<<"new size: "<<sz-1<<std::endl;
        return data[--sz];
    }
    else
        return data[0];
}

template<class T>
void Stack<T>::show() const
{
    for (int i=0; i<sz; i++) {
        std::cout<<data[i]<<" ";
    }
    std::cout<<std::endl;
}

int main(){
    Stack<int> s = {1,2,3,4,5,6,7,8,9,10,11};
    s.show();
    s.push(12);
    s.push(13);
    s.push(14);
    s.pop();
    s.pop();
    s.push(15);
    s.push(16);
    s.show();
    Stack<std::string> d = {"one","two","three"};
    d.show();
    d.pop();
    d.push("four");
    d.show();
    return 0;
}

Answer 1

Don't use memcpy to copy objects, that will copy the bits alright, but for some object a bit-wise copy is not correct as the copy constructor (or copy assignment operator) Will not be used.

A good and simple example is if you have a stack of std::string objects. When you do a bit-wise copy (with memcpy) the contents of the std::string objects are copied, but that basically is just a pointer and a size. When you do a bit-wise copy then you will have two std::string objects referencing the same memory. Destroying one of those object will lead to the other having a stray pointer to some memory (that used to contain the string) that no longer is owned by your program.

To solve this use std::copy instead to copy the objects, it will do the right thing.

---

Unrelated to your problem, but your push function does a copy that it doesn't need:

T* arr = new T[sz];
memcpy(arr, data, sz*sizeof(T));

This is simply not needed, instead do something like

T* oldData = data;
data = new T[sz + 1];

// Copy from old array to new
std::copy(oldData, oldData + sz, data);

delete[] oldData;