crash initializing size_t array

Hi guys, I'm wondering why I get a crash when initializing the indices array below (bottom)-

Code:

---

  struct TerrainGroupData
      {
        TerrainGroupData(){};
        TerrainGroupData(Ogre::Vector3 ** iVertices, unsigned long ** iIndices, size_t* iFaceNum, size_t* iVertNum)

        {

            vertices = iVertices;

            indices = iIndices;

            meshIndexCount = iFaceNum;

            meshVertexCount = iVertNum;

        }
        ~TerrainGroupData(){
            delete [] vertices;
            delete [] indices;
        };
        size_t *meshVertexCount;
        size_t *meshIndexCount;

        Ogre::Vector3 ** vertices;

        unsigned long ** indices;

      };
...
// using the struct  in cpp file
        terrainGroupData.indices = new unsigned long*[1];
        size_t size = 3;

        // resize this terrain's index buffer
        terrainGroupData.indices[0] = new unsigned long[size]; // crash here

---

Define "crash". Is it an "access violation" or "assertion failed ..." or some other message?

Its a access violation writing location 0xCCCCCCCC. I know this usually means writing to an unitialized variable but I thought that's what I'm trying to do on that line.

Quote:

---

Originally Posted by drwbns

Hi guys, I'm wondering why I get a crash when initializing the indices array below (bottom)-

---

There is no way anyone can tell you why you're getting a crash, except to say that you're mismanaging pointers and the heap. We have no idea what that "..." is doing, or whether it has any bearing on the problem. Just posting two lines that call "new" doesn't show us whether you've messed up the heap in previous operations.

One thing for sure -- if that TerrainGroupData is your entire class, it is not safely copyable, as you've made no facilities to make copies correctly, or turn off the copying altogether. It also relies on outside help to initialize those members. Why isn't that structure taking care of its own memory (allocating and deallocating), instead of you having to call "new" from outside the object to allocate the memory? What gives that object the OK to delete memory it didn't allocate itself when the object is destructed? Not only that, but this simple 1-line program will produce undefined behaviour:

Code:

---

int main()
{
  TerrainGroupData t;
}

---

That single line will cause memory corruption, or at least a heap failure. The reason is that in the destructor for TerrainGroupData, you're deleting pointers that were not initialized. If a 1-liner like that can cause trouble, imagine a more complex program.

If you want a smart pointer, where the ownership of newed memory goes to an object (which is what your code seems to be attempting to do) , then use real smart pointers such as shared_ptr and not fake it with hard-to-maintain code. Otherwise you'll be doing all sorts of gyrations and contortions trying to keep this object sound in terms of memory management. The logic to maintain this will eventually become spaghetti-like, almost similar to a "goto" issue.

You should strive to stay away from bare memory management and learn to use container classes and other higher-level constructs such as smart pointers. It is easy to type "Whatever**" or "Whatever ***", and that is what draws unsuspecting C++ coders into using these concepts. The problem is that stuff like that is very hard to maintain properly, and honestly, very little need in today's C++ programs to deal with types like that (unless it is some sort of 'C' library you're interfacing to, or you're writing your own container class).

If you're not ready to debug your own memory management issues, then you need to use other facilities that hide these details from you (and even if you did know how to handle memory, you should still use the container classes/smart pointers). The goal of your program doesn't seem to be a dynamic array class, but some sort of a game. So ask yourself, are you developing a game, or writing a dynamic array/container class? If it is the former, then use the facilities of the standard library for higher level constructs instead of using bare pointers and low-level coding. Then you can concentrate on the game you're developing and not get bogged down in data structures.

Regards,

Paul McKenzie

you said -

Quote:

---

That single line will cause memory corruption, or at least a heap failure. The reason is that in the destructor for TerrainGroupData, you're deleting pointers that were not initialized. If a 1-liner like that can cause trouble, imagine a more complex program.

---

but I'm careful to initialize all 4 struct members( the ... area) . I guess I just wanted to defer the array initializing until later in my code, which now that I look at it, I don't have to do. I can just gather the requirements to fill the array sizes, then pass that to the contructor. But I didn't think that initializing members outside the constructor would be a problem, since I have in fact done it. Thanks for the insight.

Quote:

---

Originally Posted by drwbns

you said -
but I'm careful to initialize all 4 struct members( the ... area) . I guess I just wanted to defer the array initializing until later in my code,

---

I decided to write a one line function, and boom, it fell flat on its face.

Even if it's your own code that no one else will ever run, you should write it as if there are others that will eventually need to read or maintain your code. It is very simple to create a default TerrainGroupData object even if you didn't want to do it on purpose, and then all heck breaks loose because of that bug. Nothing in that one line of code I wrote is esoteric, strange, hardly ever done, or rarely seen. It is one of, if not, the most prevalent way to create an object.

Quote:

---

But I didn't think that initializing members outside the constructor would be a problem, since I have in fact done it.

---

The issue is if it is a wise thing to do, not if you can do it or not.

Regards,

Paul McKenzie

Well, I tried to also initialize the members in the constructor, and it's still crashing. I think I'll give it a rest and try again tomorrow, thanks.

Quote:

---

Originally Posted by drwbns

Well, I tried to also initialize the members in the constructor, and it's still crashing. I think I'll give it a rest and try again tomorrow, thanks.

---

There are some other things strange about your code:

Code:

---

  ~TerrainGroupData(){
            delete [] vertices;
            delete [] indices;

---

Code:

---

        Ogre::Vector3 ** vertices;
        unsigned long ** indices;

---

You are deleting the outer dimension, but is the inner dimension also allocated? If so, this is worse than I thought, and that you are deleting only the "outside" dimension. If this is the case, where is it done or who is responsible for deleting the inside dimension? I know that you're doing this:

Code:

---

  terrainGroupData.indices[0] = new unsigned long[size];

---

so you have half the allocation deleted in the destructor, and the other half (the inner half above) deleted,..., where??.

I think you need to take a step back and think about your design. There should be no need for allocations and deallocations jumping around all over the place like this. It should be centralized in each object, or better yet, removed altogether and use container classes and smart pointers. For example, wrap those Vector3** into a class, and pass that instead of Vector3**.

Then you have this:

Code:

---

        size_t *meshVertexCount;
        size_t *meshIndexCount;

---

Can you explain why you need pointers here? What is wrong with just using "size_t"? Why overcomplicate things by using pointers?


Regards,

Paul McKenzie

The overall design was to have a loop through a number of meshes to process, and the data stored for each loop ( mesh ). I've simplified the design now based on your feedback and you brought up a lot of the quirks I noticed as well. This code was already written and I'm just trying to get it to function correctly. The "count" members don't need to be an array, like you pointed out, so I removed that. But I'm thinking maybe just create an array of the object like so -

Code:

---

TerrainGroupData* terrainGroupData = new TerrainGroupData(totalMeshes, 513)[totalMeshes];

---

but the compiler doesn't like it.
Here's my updated struct -

Code:

---

                struct TerrainGroupData
                {
                        TerrainGroupData(size_t numMeshes, size_t mapSize){
                                vertices = new Ogre::Vector3[mapSize*mapSize];
                                indices = new unsigned long[((mapSize*mapSize)-(mapSize*2)) * 6];
                        };
                        TerrainGroupData(Ogre::Vector3 * iVertices, unsigned long * iIndices, size_t iFaceNum, size_t iVertNum)

                        {

                                vertices = iVertices;

                                indices = iIndices;

                                meshIndexCount = iFaceNum;

                                meshVertexCount = iVertNum;

                        }
                        ~TerrainGroupData(){
                                delete [] vertices;
                                delete [] indices;
                        };
                        size_t meshVertexCount;
                        size_t meshIndexCount;

                        Ogre::Vector3 * vertices;

                        unsigned long * indices;

                };

---

Quote:

---

Originally Posted by drwbns

The overall design was to have a loop through a number of meshes to process, and the data stored for each loop ( mesh ). I've simplified the design now based on your feedback and you brought up a lot of the quirks I noticed as well. This code was already written and I'm just trying to get it to function correctly. The "count" members don't need to be an array, like you pointed out, so I removed that. But I'm thinking maybe just create an array of the object like so -

Code:

---

TerrainGroupData* terrainGroupData = new TerrainGroupData(totalMeshes, 513)[totalMeshes];

---

---

You can't use array "new" to construct anything except by default. Since your class requires arguments to the constructor, new[] isn't going to work.

Your class also is not safely copyable, since you don't have a user-defined copy/assignment operator defined to handle the dynamically allocated memory held by those pointers. Also, your calculation for the number of indices seems to be off in the first constructor (it seems to equal 0 indices).

Try this:

Code:

---

#include <vector>
typedef std::vector<Ogre::Vector3> Vertex3Array;
typedef std::vector<long> IndexArray;

struct TerrainGroupData
{
    TerrainGroupData(size_t numMeshes, size_t mapSize) : vertices(mapSize *2)
    { }

    TerrainGroupData(const Vertex3Array& iVertices, const IndexArray& iIndices, size_t iFaceNum, size_t iVertNum) : vertices(iVertices), indices(iIndices), meshIndexCount(iFaceNum), meshVertexCount(iVertNum)
    { }

    size_t meshVertexCount;
    size_t meshIndexCount;
    Vector3Array vertices;
    IndexArray indices;
};

---

Usage of this class now will not crash. unless your parameters that are size_t are negative numbers. The issue of deletion has also been solved (note that no destructor is needed). Also note the usage of the initialization list in the constructor functions.

If you replaced your class with this one, recompiled, and fix the compiler errors, then the code should more than likely work, or at the very least, not crash. The only compiler errors would be related to using vector instead of an array, but those are easily fixed since a vector is just a wrapper for an array you would create using "new[]". In other words, vector does exactly the same thing you were doing, but safely, efficiently, and without errors, and you don't need to any cleanup due to vector's destructor.

To access the array inside of a vector so that "old" functions recognize that the item is an array, just send the address of the first item in the vector:

Code:

---

void SomeFunc(Ogre::Vertex3 * pVertices, int numElements)  // function that assumes that this is an array of Vertex3 with numElements entries
{
//...
}

//..
Vertex3Array v(10);  // this is actually a vector of 10 Oger3::Vertex3 elements.
SomeFunc(&v[0], v.size());  // calls the function that thinks it is using a Vertex3 pointer to access the 10 elements.

---

Regards,

Paul McKenzie