Problem with malloc and memcpy in array class

Question

I've started writing some code for a List class and I'm lost. I need it for my Arduino project – I can't use STL.

Here's the code.

#include <iostream>
    
template <typename T>
class List
{
public:
    List<typename T>()
    {
        m_Count = 0;
        m_Data = nullptr;
    }
    
    ~List()
    {
        free(m_Data);
    }
    
    void Push(const T& element)
    {
        m_Count++;
        T* alloc = (T*)malloc(size_t(sizeof(T) * m_Count));
        memcpy(alloc, m_Data, m_Count * sizeof(T));
        *(m_Data + sizeof(T) * (m_Count - 1)) = element;
    }
    
    T* operator [](unsigned int x) const
    {
        return (m_Data + x * sizeof(T));
    }
    
private:
    T* m_Data;
    uint64_t m_Count;
    
};
    
struct Vertex
{
    int x;
    int y;
};
    
int main()
{
    List<Vertex> list;
    list.Push({ 0, 1 });
    list.Push({ 2, 3 });
    list.Push({ 4, 5 });
    
    std::cout << list[0]->x << list[1]->x << list[2]->x;
}

The problem lies somewhere in the Push method: When I call memcpy, the program triggers a compiler breakpoint.

Answer 1

The essential problem is in the line *(m_Data + sizeof(T) * (m_Count - 1)) = element;. Here, you are attempting to copy the given element into the old (i.e. pre-existing) m_Data array; this will be nullptr the first time the Push function is called (and one element too small every other time)^†.

So, you need to first release the old data (with free(m_Data)), then assign your newly-allocated memory to that pointer (m_Data = alloc), and only then copy element to that array's last element.

However, as others have said, why are you using malloc and free in C++? The code below replaces those calls with new[] and delete[] (although using a std::vector would likely be easier/better/safer, if that were possible).

#include <iostream>

template <typename T>
class List {
public:
    List<T>() { // Don't need "typename" here!
        m_Count = 0;
        m_Data = nullptr;
    }
    ~List() {
        delete[] m_Data;
    }
    void Push(const T& element) {
        m_Count++;
        T* alloc = new T[m_Count];
        for (uint64_t i = 0; i < m_Count - 1; ++i) alloc[i] = m_Data[i]; // Copy old data (if any)
        delete[] m_Data; // Release old data
        m_Data = alloc; // Assign newly-allocated memory to m_Data
        m_Data[m_Count - 1] = element; // Why use pointer arithmetic when you have the [] operator?
    }
    T* operator [](unsigned int x) const {
        return &m_Data[x];
    }

private:
    T* m_Data;
    uint64_t m_Count;

};

struct Vertex {
    int x;
    int y;
};

int main()
{
    List<Vertex> list;
    list.Push({ 0, 1 });
    list.Push({ 2, 3 });
    list.Push({ 4, 5 });
    std::cout << list[0]->x << list[1]->x << list[2]->x << std::endl;
    std::cout << list[0]->y << list[1]->y << list[2]->y << std::endl;
    return 0;
}

I have made a couple of other 'minor improvements' to your code (your operator [] looked very suspicious, as the size of the pointed-to object is inherently taken into account when doing pointer arithmetic); there are others that could be made but that would, IMHO, deviate too far from the code you posted.

---

^† Actually, it will always be nullptr in your code, as you never assign anything else to it.