list class is crashing in the destructor, why?

Question

The following code uses placement new to move each value. The copy constructor and operator = have been deleted, so the only place where new memory is allocated, and old is copied is in the add function.

#include <iostream>
#include <string>
using namespace std;

template<typename T>
class DynArray {
private:
  T* arr;
  uint32_t size;
  void* operator new(size_t size, void* loc) {
    return loc;
  }
public:
  DynArray() : arr(nullptr), size(0) {}
  ~DynArray() {
     delete [] arr;      
  }
  DynArray(const DynArray& orig) = delete;
  DynArray& operator =(DynArray copy) = delete;
  DynArray(DynArray&& orig) : arr(orig.arr), size(orig.size) {
    orig.arr = nullptr;
  }

  void add(const T& v) {
    const T* old = arr;
    arr = (T*)new char[(size+1)*sizeof(T)];
    char* temp = (char*) arr;
    for (uint32_t i = 0; i < size; i++, temp += sizeof(T))
      new(temp) T(old[i]); // this SHOULD do a move?
    new (temp) T(v);
    size++;
    delete [] (char*)old; // don't call destructors for strings, they are moved
  }
  friend ostream& operator <<(ostream& s, const DynArray& list) {
    for (uint32_t i = 0; i < list.size; i++)
      s << list.arr[i] << ' ';
    return s;
  }
};

DynArray<string> f(int n) {
  DynArray<string> ans;
  for (int i = 0; i < n; i++)
    ans.add("ab");
  return ans;
}

int main() {
    DynArray<string> c;
    c.add("x");
    c.add("y");
    c.add("zzzz");
    cout << c << '\n';

    DynArray<string> d = f(3); // calls move constructor
    cout << d << '\n';
    // code crashes in destructor of d, why>
}

What is the source of the problem?

Answer 1

You delete[] a T* pointer that was allocated as new char[...]: The wrong type.

You must delete it as its original type (i.e., delete[] reinterpret_cast<char*>(arr)).

And before that you must manually call the destructor of each value. Moved-from objects also still have to be destroyed:

  ~DynArray() {
    // Destroy in reverse order of construction
    for (T* p = arr + size; p != arr;) {
      (--p)->~T();
    }
    delete [] reinterpret_cast<char*>(arr);      
  }

  void add(const T& v) {
    // ...
    for (T* p = old + (old_size); p != old;) {
      (--p)->~T();
    } 
    delete [] reinterpret_cast<char*>(old);
  }

You should probably replace new char[size]/delete[] ptr with ::operator new(size)/::operator delete(ptr) or std::allocator_traits<std::allocator<T>>::allocate(alloc, size)/std::allocator_traits<std::allocator<T>>::deallocate(alloc, ptr, size).

Also new(temp) T(old[i]) does not move. new(temp) T(std::move(old[i])) would move. You do need to think about exception safety: If one of these constructors throws, you need to destroy the previously constructed objects. Or while the objects are being constructed in the new list, the list itself is in an unstable state, which is bad if a move constructor accesses the list. Your operator new function is also unnecessary.

And finally, if this is not a learning exercise, just use std::vector<T>