Pushing pointers into c++ vectors, and cleanup

Question

I am converting some code between different systems, and I have a question regarding c++ vectors.

If I do something like this:

In header file:

struct Vertex
{
    float x;
    float y;
    float z;
}

struct submesh
{
    Vertex *meshdata;
}

std::vector<submesh> meshes;

In a routine in the c++ file:

{
    Vertex *data = new Vertex[1024];

    submesh g;
    g.meshdata = data;
    meshes.push_back(g);

    delete [] data;
}

Will I be in trouble? My assumption is that the vector would hold a pointer to data that is no longer valid once I called delete on it. Do I need to write a copy constructor for Vertex so that the data is copied first?

Additional:

The question was more to do with how do I put a pointer to allocated memory into a std::vector<> and still cleanup the locally allocated data. Essentially, how do I copy the data into the vector so I can still clean up my copy.

The original code was in DirectX. I am porting it to the iPhone. The original code allocated a submesh locally in a routine using:

{
    ID3DXMesh* subMesh = 0;
    D3DXCreateMesh(SubGrid::NUM_TRIS, SubGrid::NUM_VERTS, D3DXMESH_MANAGED, elems, gd3dDevice, &subMesh));

    //
    // ... do some magical things to submesh
    //

    SubGrid g;
    g.mesh = subMesh;
    g.box  = bndBox;
    mSubGrids.push_back(g);
}

I am trying to duplicate how ID3DXMesh is able to be added to a vector, then lose it's scope in the routine.

As I don't have access to D3DXCreateMesh(), I figured I would simply allocate the vertices I needed, throw them into a vector, and clean up.

Sorry, I wanted to keep the nitty gritty details out of it, as the question is simply how do I allocate a chunk of data, put a pointer into a std::vector<>, then clean up the locally allocated memory. :)

I assumed a copy constructor had to be written somewhere. Just wasn't sure where or how.

A subgrid looks like this:

struct SubGrid
{
    ID3DXMesh* mesh;
    AABB box;

    // For sorting.
    bool operator<(const SubGrid& rhs)const;

    const static int NUM_ROWS  = 33;
    const static int NUM_COLS  = 33;
    const static int NUM_TRIS  = (NUM_ROWS-1)*(NUM_COLS-1)*2;
    const static int NUM_VERTS = NUM_ROWS*NUM_COLS;
};

And the vector they get added to looks like:

std::vector<SubGrid> mSubGrids;

Answer 1

Don't directly dynamicly-allocate when you don't need to, and in this case you don't. Since you're filling your own submesh data rather than using ID3DXMesh, the container of that data should be RAII-compliant. If I were coding this I would remove the submesh class entirely and just use:

// vector containing list of vertices.
typedef std::vector<Vertex> SubMesh;

Your SubGrid class can then become a simple container that holds, as one of its properties, a submesh collection. I noticed you also have a class AABB for a box object. You would continue to keep that inside SubGrid. I don't have ton to work with here, so I'm making some of these up as I go along, but something like the following:

// a simple 3-value triplet of floats
struct Vertex
{
    float x,y,z;
};

// a Submesh is an arbitrary collection of Vertex objects.
typedef std::vector<Vertex> SubMesh;

// I'm defining AABB to be an 8-vertex object. your definition
//  is likely different, but I needed something to compile with =)
typedef Vertex AABB[8];

class SubGrid
{
public:
    SubGrid() {};

    // comparator for container ordering
    bool operator <(const SubGrid&);

    // submesh accessors
    void setSubmesh(const SubMesh& mesh) { submesh = mesh;}
    SubMesh& getSubmesh() { return submesh; }
    const SubMesh& getSubmesh() const { return submesh; }

    // box accessors
    AABB& getBox() { return box; }
    const AABB& getBox() const { return box;}

private:
    SubMesh submesh;
    AABB box;
};

// arbitrary collection of SubGrid objects
typedef std::vector<SubGrid> SubGrids;

When adding this to your global SubGrid collection g, you have several possibilities. You could just do this:

// declared globally 
Subgrids g;

// in some function for adding a subgrid item
SubGrid subgrid;
AABB& box = subgrid.getBox();
SubBesh& submesh = subgrid.getSubmesh();

// ... initialize your box and submesh data ...

g.push_back(subgrid);

But you'd be copying a lot of data around. To tighten up the memory access you could always do this instead:

// push an empty SubGrid first, then set it up in-place
g.push_back(SubGrid());
Subgrid& subgrid = *(g.back());
AABB& box = subgrid.getBox();
SubMesh& submesh = subgrid.getSubmesh();

//... initialize your box and submesh data ...

This will establish a reference to the SubGrid just added to the global collection, then allow you to modify it in-place. This is but-one of a number of possible setup options. It should be noted that if you have C++11 in your toolchain (and if you're doing this on MacOS or iOS, you likely do, as Apple LLVM 4.2's clang is pretty good on C++11 compliance) this can get even more efficient with judicious usage of move-constructors and move-assignment-operators.

Most importantly, not a new or delete to be seen.

Anyway, I hope this gives you some ideas.

Answer 2

Your code looks fine in single threaded application. Your code only allocate data memory once and delete [] data once.

Do I need to write a copy constructor for Vertex so that the data is copied first?

Your code is clean as you shown, meshes points to only allocated data. If you meant to copy data when call meshes.push_back(g), then your code doesn't do what you meant to.

You might want to use std::vector instead:

struct submesh
{
    std::vector<Vertex> meshdata;
}

vector<submesh> meshes;

void Func()
{
    meshes.emplace_back(submesh());
    meshes.at(0).meshdata.resize(100);
}

STL container uses RAII idiom, it manages memory deallocation for you automatically.

Answer 3

Yes of course, vector will have a pointer to deleted memory. What you need is either:

1. Create copy constructor for submesh (not Vertex).OR

2. Changesubmesh to have array of Vertex (not just a pointer).

Copy constructor can be done like this:

struct submesh
{
    Vertex *meshdata;
    unsigned meshsize;
    submesh(Vertex* v = 0, unsigned s= 0) : meshdata(v), meshsize(s){}
    submesh(const submesh& s)
    {
        if(meshdata) /*we have stored data, delete it.*/ delete(meshdata);
        meshdata = new Vertex[s.meshsize];
        meshsize = s.meshsize;
        memcpy(meshdata, s.meshdata, sizeof(Vertex) * meshsize);
    }
};

For sure it is much recommended to use unique_ptr (if you use c++11) or auto_ptr for old c++. To avoid the nightmare of memory management as much as you can.

Check How to avoid memory leaks when using a vector of pointers to dynamically allocated objects in C++?