CODEWITHSUNDEEP
c++pointersmemory-management
Broxzier
Broxzier

Reputation: 2949

Memory Deallocating freezes my program

This is not a problem, just a question that I want to be answered.

I'm making 2D application that has particles. In the click handler I've written this code:

Particle *tempp = new Particle();
tempp->setPosition(mx, my);
particles.push_back(tempp); // typeof particles = std::list<Particle*>
delete tempp; // <- this line is the problem

When I click, one particle will be created at the mouse position. After about one second it should disappears, which works fine. After it's disappeared I can click again to create a new particle.

However, when I click while there still is one particle on the screen, my program freezes and stops working.

My destructor of the Particle class and it's parent's destructor are both empty.

WITHOUT calling delete my program runs fine, even with multiple particles at once, or even multiple per frame. I am just wondering what is causing this freezing issue.

Upvotes: 2

Views: 1920

Answers (5)

Shafik Yaghmour
Shafik Yaghmour

Reputation: 158559

You are putting a pointer into the container and then deleting it which is a problem. push_back will copy the value of the pointer not the contents of the pointer so when you call delete, the pointer in the container is no longer valid. So now you have a dangling pointer and when you dereference it this will be undefined behavior but most likely a crash.

Upvotes: 2

Kate Gregory
Kate Gregory

Reputation: 18964

When I see new and then a few lines later delete, I would rather see the stack used. Unless Particle is enormous (which I doubt) you could change your code to this:

Particle tempp;
tempp.setPosition(mx, my);
particles.push_back(tempp); // change particles to std::list<Particle>

Presto. You write less code and you won't blow up.

Upvotes: 3

enhzflep
enhzflep

Reputation: 13109

The solution I believe, lies in your 3rd line of code. Note that particles is a vector of pointers to Particles? Well, in the 3rd line, you create a copy of the pointer and insert it into a list. On the very next line, you deallocate the memory that is pointed-to by that pointer. The list does not store your particle - it merely stores the memory address of the particle So, when you delete the particle, you tell the compiler to re-use the memory that contains your valid data.

So, while commenting the 4th line makes the problem go away, it's not actually crashing there - there's no reason for it to. The likely scenario is that it crashes when trying to do something with the memory that used to be yours, but was freed in line 3.

You could make the list store actual Particles as a fix.

Upvotes: 2

Some programmer dude
Some programmer dude

Reputation: 409364

When you push the pointer into the list, you only push a copy of the actual pointer and do not make a copy of what it points to. This means that after the push_back you have two pointers pointing to the same memory.

If you then free that memory, then you have a pointer pointing to free memory, and that pointer is now invalid.

Upvotes: 3

hmjd
hmjd

Reputation: 122001

Based on the posted code, the particles container will contain dangling pointers. Any attempt to dereference these is undefined behaviour. I am assuming they are later being used, otherwise the storage of them seems pointless.

Calling push_back() does not copy the pointed-to-object but copies the value of the pointer (the memory address of the dynamically allocated object). If Particle is cheap to copy, is copyable and polymorphic behaviour is unrequired just store the Particle in the container. Otherwise, suggest using a smart pointer, such as std::unique_ptr, to automatically destruct the Particle when removed from the container.

Upvotes: 5

Related Questions