CODEWITHSUNDEEP
c++dynamicdynamic-memory-allocation
Darestium
Darestium

Reputation: 653

Error, dynamically allocating objects to an array

I have a pointer to an array of structs like so:

class Terrian  {
     ...
    private:
        Vector *terrian_vertices;
     ...
}

And the data for the pointer is generated in the "construct_vertices" function

Terrian::Terrian(int width, int height)  {
    this->width = width;
    this->height = height;

    std::cout << "Width: " << width << "  Height: " << height << "\n";

    std::cout << "Vertices\n";
    construct_vertices();
    std::cout << "Element\n";
    construct_elements();
    std::cout << "Buffers\n";
    construct_buffers();
}

void Terrian::construct_vertices()  {
    terrian_vertices = new Vector[width * height];

    std::cout << "Generating data\n";

    for (int x = 0; x < width; x++)  {
        for (int y = 0; y < height; y++)  {
            int index = x + y * width;

            Vector *pos = new Vector((GLfloat)x, 0.0f, (GLfloat)-y);
            memcpy(pos, terrian_vertices, sizeof(Vector) * index);

            std::cout << terrian_vertices[index].x;

            Color *color = new Color(0, 255, 0);
            memcpy(color, terrian_colors, sizeof(Color) * index);
        }
    }
}

Here is the program's output (all I do in the main function is instantiate the object)

Width: 32  Height: 32
Vertices
Generating data
5.2349e-039
Process returned -1073741819 (0xC0000005)   execution time : 10.073 s
Press any key to continue.

The program crashes when the first pointer is copied to the array, and the output for 'x' should be 0. Which is puzzling. Does anyone know what is causing this to happen? If so, is there a better way to allocate structs dynamically - without using memcpy?

Upvotes: 0

Views: 283

Answers (2)

R. Martinho Fernandes
R. Martinho Fernandes

Reputation: 234474

Does anyone know what is causing this to happen?

The use of memcpy is incorrect. Any reference documentation will tell you that.

The first parameter is a pointer to the destination, which would be index elements into the terrian_vertices array: terrian_vertices + index.

The second parameter is a pointer to the source, which is pos.

(If you're curious, the reason the destination comes before the source is because it parallels the assignment operator: destination = source)

The third parameter is the amount of data to copy, which in your case would just be sizeof(Vector): it's just one Vector it needs to copy, not index.

Misusing memcpy like the code does easily leads to undefined behaviour, which is luckily manifesting as an error.

If so, is there a better way to allocate structs dynamically - without using memcpy?

Yes. Don't manage memory yourself: use std::vector and normal copy semantics.

class Terrian  {
// ...
private:
    std::vector<Vector> terrain_vertices;
    // Hmm, this may need some touch up on naming,
    // or it may get confusing with two "vector" thingies around
};

// ...

void Terrian::construct_vertices()  {
    terrain_vertices.reserve(width * height);
     // reserve is actually optional,
     // but I put it here to parallel the original code
     // and because it may avoid unneeded allocations

    std::cout << "Generating data\n";

    for (int x = 0; x < width; x++)  {
        for (int y = 0; y < height; y++)  {
            terrain_vertices.emplace_back((GLfloat)x, 0.0f, (GLfloat)-y);
            // or this if your compiler doesn't support C++11:
            // terrain_vertices.push_back(Vector((GLfloat)x, 0.0f, (GLfloat)-y));

            std::cout << terrian_vertices[index].x;

            // same thing for colors
            terrain_colors.emplace_back(0, 255, 0);
        }
    }

Notice how now there's no new anywhere in sight. This solves another issue with the original code: it was leaking one instance of Vector and one of Color per loop iteration.

Upvotes: 5

Ed Swangren
Ed Swangren

Reputation: 124642

Vector *pos = new Vector((GLfloat)x, 0.0f, (GLfloat)-y);
memcpy(pos, terrian_vertices, sizeof(Vector) * index);

You can't do that. new allocated enough memory for a Vector and pos points there. However, you then proceed to copy sizeof(Vector) * index bytes into that location. The first time this ends up being 0 bytes because int index = x + y * width; is 0. Next time it is 2 * width, which is likely greater than1, so you end up copying past*pos` into no man's land.

On a side note, you shouldn't be using memcpy to copy complex types. It may be ok and a bit for bit copy may be all that is needed, but this could go bad for you if used with some types (i.e., types which cannot be bit copied due to internal semantics, like RAII style containers).

Upvotes: 0

Related Questions