Sign in to follow this  
rikardo

Delete memory from inside a class

Recommended Posts

MyClass* a = new MyClass;

...

delete a;
a = 0;
The code above would allocate memory for my object 'a', and later bring it back. Say this instead:
class MyClass {
private:

    ...

public:

    MyClass() {
        ...
    }

    ~MyClass() {
        ...
    }

    void destroy() {
        delete this;
    }
};
And now:
MyClass* a = new MyClass;

...

a -> destroy();
Would the memory allocated for 'a' now be released?

Share this post


Link to post
Share on other sites
I tried it and the data (I just put an int in) was 0xfeeefeee (implying its been deleted (well with visual studio)).

Its a really "bad" thing to do though, why would you do this instead of just using delete? Is this just out of curiosity or were you actually planning on using it?

Share this post


Link to post
Share on other sites
While it's legal, this is very bad mojo :) It goes against my "if you do, you must undo" guideline (even though there's discussion on the topic).

(a) The object did not allocate its own memory (something else did, and then called its constructor), and so should not be responsible for deallocating it.

(b) As a C++ programmer if I see a "new" somewhere I expect to see an accompanying "delete" as well. I certainly don't expect to call "destroy" on an object if I "new"'d it somewhere else.

(the above is in addendum to the usual risks and cautionary words as they apply to suicidal classes, not a replacement)

So yeah - it will work, the memory will be deallocated as you expect, and is perfectly legal .. but it's bad mojo =)

Share this post


Link to post
Share on other sites
All objects created with this "self destructive" behaviour are added to a manager. So you can say that you give all the resposibility to the manager when its created. It is the base object of my gui that uses this destroy method in order to remove ALL the children that is attached to it. So codewise it looks something like this:

gui::Window* window = new Window;
window -> setSize(400, 300);
window -> setPosition(100.0, 300.0);
...
manager.add(window);
...
// Main loop.
while (1) {
...
manager.update();
}


So say that you hit the close button on the window object and have to do something like this:

while (1) {
if (window -> closeButtonIsClicked()) {
delete window;
}
...
}


Instead the manager has a list with all the frames and when the close button is clicked it gets a message that it should DESTROY the frame. When it does it just calls the window's destroy method and removes it from the list.

Share this post


Link to post
Share on other sites
It sounds like your putting a lot of effort to get around behavior C++ already has built in aka the destructor.

You could replace the pointers with smart (strong and weak) pointers, which provides a much safer basis for deleting the allocated memory automatically. You just need to put all of your destruction code into the destructor.

Like what you said in your example, you want the object's children to be deleted as well. Your class can store strong pointers to the children (ensuring their lifespan till the parents deletion) and the children have weak pointers to the parent (ensuring the children don't accidently keep a parent alive).

With the reference counting provided by the smart pointer, your resource manager can see if anything other than itself has a strong pointer to the class. If it they don't and you want to destroy the object, all you have to do is basically pop it out of the resource manager. When no more objects have a copy of the smart pointer, all of the destructor code gets calls. In the destructor is where you can have the class perform the destruction of the children.

[edit]
You can even still create a seperate function from the destructor that performs all of the cleanup, without actually calling delete this. This would allow you to call it whenever you want, plus not duplicate code if you just want the destuctor to do it too (by adding a call to it in the destructor).

Share this post


Link to post
Share on other sites
Quote:
Original post by rikardo
Instead the manager has a list with all the frames and when the close button is clicked it gets a message that it should DESTROY the frame. When it does it just calls the window's destroy method and removes it from the list.


Well, let me ask you this - what would you gain from calling "obj->destroy()" instead of "delete obj" and still provide the same functionality?

Putting it differently: if I got a hold of your code and removed the "destroy()" function completely and replaced all references to that function with "delete ptr;" .. what would be lost/broken in the process?

Share this post


Link to post
Share on other sites
Understand your view, but the object which invokes the destroy method looks somthing like this:

void destroy() {
for all children {
currentChildren -> destroy();
}

delete this;
}


If you remove this code it will not destroy all the children. And if you try to put this code in the destructor then a delete this call will produce a infinite loop, since after the delete call, the destructor is called.

Share this post


Link to post
Share on other sites
So, replace it like so:

Whatever::~Whatever()
{
for(Child *child in children)
{
delete child;
}
}


The caller uses delete instance; instead of instance->destroy()

And hey presto! Its idiomatic and works perfectly. Now, add smart pointers to the above and you save yourself a host of headaches.

Share this post


Link to post
Share on other sites
Now it's more a matter of taste since this behaviour is hidden from the one that uses the code. But I would agree that a new would somwhere in the open be followed by a delete and not hidden.

Share this post


Link to post
Share on other sites
The reason we are suggesting changing how you are doing it, is because it is much safer and takes advantage of RAII.

With smart pointers, you can guarentee the destructor is called when there are no longer any references to your object. And thus can also guarentee the children's deletion, assuming nothing else has a smart pointer to them as well.

Since you are deleting the object internally, you might not know what is still trying to use that pointer. Internally it might be deleted, but externally there could be other code that don't realize the object has been deleted and will still try and access the object/memory like it was still there. The smart pointers help prevent this from happening.

Share this post


Link to post
Share on other sites
Quote:
Original post by rikardo
Now it's more a matter of taste since this behaviour is hidden from the one that uses the code. But I would agree that a new would somwhere in the open be followed by a delete and not hidden.

It is clearer to the average programmer what is going on when you explicitly use delete. The question is, what advantages does wrapping this behaviour in a function have? A disadvantage is that it can leak memory if one uses naked delete instead of the "destroy" function.

Share this post


Link to post
Share on other sites
Quote:
Original post by rikardo

Instead the manager has a list with all the frames and when the close button is clicked it gets a message that it should DESTROY the frame. When it does it just calls the window's destroy method and removes it from the list.


The problem is that you need to guarantee that nobody holds the reference to your just destroyed window. This is non-trivial.

One way is to use boost's shared_ptr and enable_shared_from_this. It may however leave dangling pointers.


Alternative can use deferred destruction. Something like this:

...
if (window -> closeButtonIsClicked()) { manager.mark_for_destruction(this); }
...

while (1) {
manager.update();
manager.cleanup();
}
This is poll-based garbage collection.

cleanup() is implemented something like this:

for (every object d in marked_for_destruction)
for (every object o in UI)
o.remove_object(d);
delete d;
}
This ensures that no dangling pointers remain in UI graph, but it cannot prevent dangling pointers from arbitrary code. This might be a good idea even with smart pointers since it can be used to break cycles, which are not uncommon in UI.

For this reason one would need to make pointer to UI objects some special handle which discourages or tries to prevent code referencing it, at which point smart pointers are a much more robust solution anyway.


Any particular reason for rolling your own UI framework instead of existing ones? Or at least reusing existing architectures (MVC + observers)?

Share this post


Link to post
Share on other sites
Quote:
Original post by Grafalgar
Quote:
Original post by rikardo
Instead the manager has a list with all the frames and when the close button is clicked it gets a message that it should DESTROY the frame. When it does it just calls the window's destroy method and removes it from the list.


Well, let me ask you this - what would you gain from calling "obj->destroy()" instead of "delete obj" and still provide the same functionality?

Putting it differently: if I got a hold of your code and removed the "destroy()" function completely and replaced all references to that function with "delete ptr;" .. what would be lost/broken in the process?
The practice of deleting an object from inside its own member function is at the heart of all intrusive reference-counting schemes, e.g. the one used in COM.

Share this post


Link to post
Share on other sites
Quote:
Original post by iMalc
The practice of deleting an object from inside its own member function is at the heart of all intrusive reference-counting schemes, e.g. the one used in COM.


Sure, but not in his case. Since he's not doing any reference counting (from what I can tell), deleting the memory internally (as opposed to simply calling "delete ptr") does not allow him to solve any particular issues that would otherwise be much more difficult/error-prone if using only "delete."

[Edited by - Grafalgar on December 23, 2009 12:11:30 PM]

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this