Printable View
I made a linked list of dynamic objects, and a function to call them starting from the first one.
I need to destroy a dynamic object using "this" pointer, or some other way...
the problem is that if I call the destructor from inside of the class (some member function), it works, but causes a memory leak, if i call "delete this;" the program crashes.
I'm a still a n00b in c++...
thanks
It is ALMOST never right to call "delete this;".
To find the correct way to do things, you probably would have to post (some of) your code.
--
Mats
The nodes you allocate using new should be deleted via delete. That simple.
Delete this is dangerous and should only be considered in special situations.
But even better is using smart pointers. That will make sure they get deleted without a memory leak when the time is right.
My previous company did that all over the place. Each of their objects contained a Delete() function which just did delete this;Quote:
Originally Posted by matsp
I never saw any problems when using it.
What other situations of delete this; are actually wrong and why?
The only bad use I can think of right now would be trying to delete an object that wasn't dynamically allocated.
Yep, deleting objects on stack and the problem that it can leave a dangling pointer, if you aren't careful.
I don't think it's that bad, but you should definitely be careful with it. There are some situations where it's helpful.
For further reading: Is it legal (and moral) for a member function to say delete this?
everything exept the deleting works. when I try to call the destructor, it just simply removes the linking to other obj. but does not deallocate the memory.
"delete this;" wont work, and I don't know why...
here's the code of my program:
main.cpp
Code:#include <iostream>
class dyn_obj
{
public:
dyn_obj();
~dyn_obj();
void function(int &);
dyn_obj *prev, *next;
private:
bool del_now;
};
dyn_obj *first_obj = 0, //first object's mem address
*Prev_obj = 0; //the current object's address
dyn_obj::dyn_obj():del_now(false)
{
next = 0;//set the link to the next object to 0, there is no next object yet
if( first_obj == 0 )//if this is the first object in the list
{
first_obj = this;//set this as the first object
prev = 0;//set prev to 0, this are no previous objects.
}
else//if NOT the first object
{
prev = Prev_obj; //link this object to the previous object
prev -> next = this;//link the previous object to this object
}
Prev_obj = this;
}
dyn_obj::~dyn_obj()
{
if(next == 0)
{
if(prev == 0)//if this is the only object existing
{
first_obj = 0; //set first object to 0, no first object
}
else//if this is the last object in the list
{
prev -> next = 0; //remove the linking from the object before
}
}
else
{
if(prev == 0)//if this is the first object in the list
{
first_obj = next; //set the next object as the first one
next -> prev = 0;
}
else//if this object is somewhere in the middle of the list
{
prev -> next = next;//link the previous object to the next one
next -> prev = prev;//and vice versa
}
}
}
void dyn_obj::function(int & value)
{
value++;
if(del_now)delete this;
else del_now = true;
}
int call()
{
int n = 0;
if( first_obj != 0 )//if the list is not empty
{
dyn_obj *object = first_obj;//create a pointer to the first object in the linked list
while( true )
{
object -> function(n);//call the object
if(object -> next != 0) object = object -> next;//if not last object, move to the next one
else break;
}
}
return n;
}
int main()
{
for(int n = 0; n < 10; n++) new dyn_obj;
std::cout << "10 objects created. let's call them." << std::endl;
std::cout << "the function \"function\" was called " << call() << " times." << std::endl;
std::cout << "let's call the functions a second time, making the objects delete themselves." << std::endl;
call();
std::cout << "deleted.";
std::cin.get();
}
This implementation will confuse and creates bugs. That's all.
Reconsider it to a more true OOP implementation.
I would personally like to see a "holder" (aka container) object for your dyn_obj's - this would have the pointers "first_obj" and "prev_obj" [which traditionally are called head and tail respectively, but that's your choice - your names are certainly better than "a" and "b"!].
Then let the holder deal with deleting the objects.
Your current construction makes the whole thing a bit convoluted.
I don't see any reason why the current design doesn't de-allocate the memory, however.
--
Mats
Not to mention - that removing node from the middle of the 2-linked list requires to update 4 pointers and not 2
yes, i'd like to make an another class where to place that call() function, the *first_obj and *prev_obj pointers and make it "parent" to the dyn_obj class, but when I tried I got like 40 errors, and posted that one, I still need to learn the class inheritance and some other important stuff.
There should be no need for inheritance for such a class - one class CONTAINS the other class, but they are not related, just like the class below is not inheriting "string":
--Code:class person
{
string name;
...
public:
const string &getName() { return name; }
...
}
Mats
4? two of them will be destroyed...Quote:
Originally Posted by vart
ok it's always good practise :)
Oh you silly programmers with your hyperboles.Quote:
Originally Posted by laserlight
I would like to see something along the lines of this:
CLinkedListBeginOrEnd is optional, but is useful is you want your lists to have a head and tail pointer.Code:template<typename Type> class CLinkedList
{
public:
//CLinkedList(): pNext(NULL), pPrevious(NULL), pObject(NULL), pBeginOrEnd(NULL) { }
CLinkedList(CLinkedListBeginOrEnd<Type>* pBeginOrEnd): pNext(NULL), pPrevious(NULL), pObject(NULL), pBeginOrEnd(pBeginOrEnd) { }
//CLinkedList(const CLinkedList<Type, Allocator>& rCopy);
void InsertAfter(CLinkedList<Type>* pAfter);
void InsertBefore(CLinkedList<Type>* pBefore);
void InsertAtBeginning();
void InsertAtEnd();
void Remove();
//void Initialize(CLinkedListBeginOrEnd<Type, Allocator>* pBeginOrEnd);
//CLinkedList& operator = (const CLinkedList<Type, Allocator>& rCopy);
Type* pObject;
private:
CLinkedList* pNext;
CLinkedList* pPrevious;
CLinkedListBeginOrEnd<Type>* pBeginOrEnd;
};
Implementation is missing on purpose.
Perhaps you might get an idea of what is possible via linked lists in C++...
when I try to build that code withQuote:
Originally Posted by matsp
-runtime error,Code:void dyn_obj::function(int & value)
{
value++;
if(del_now)delete this;
else del_now = true;
}
and
-works, but looking at the task manager: it doesn't deallocate the memory(objects to create set to 1 000 000, after the second callCode:void dyn_obj::function(int & value)
{
value++;
if(del_now)this -> ~dyn_obj();
else del_now = true;
}
the list is not linked, but the memory consumption is the same)
What runtime error do you get for the first code.
Also de-allocating memory doesn't necessarily mean that the application's used memory goes down - it only means that it won't grow again. So using task manager to determine if memory is actually freed or not is a bad plan. Although I certainly believe that if you don't call delete it doesn't get freed - it's obvious, really.
--
Mats
A destructor does not deallocate memory. A destructor is merely called before the memory is deallocated or the object goes out of scope.
OK I think that's complete enought :DQuote:
Originally Posted by Elysia
thanks Elysia
I don't know very mutch about STL, how fast are the functions in it compared to a pro -made linked list, when the goal is to create,
call and destroy a lot of objects in a short amount of time?
ok, so calling the destructor is a reallllly bad idea...Quote:
Originally Posted by Elysia
The overall speed depends on the implementation, but you can be sure they are plenty fast.Quote:
Originally Posted by cminusminus
Yes... it is. You should use an object after having called the destructor.Quote:
Originally Posted by cminusminus
what's a good tool to do that, does the standard VC++ debugger have something (easy) for that purpose?Quote:
Originally Posted by matsp
I'm not sure there is one good tool for this. I have been working on non-Windows systems for the last several years, and before that I was working on Windows graphics drivers. In most instances of that, it's a case of "custom built" memory tracker - which doesn't help you.
A trivial test would be to create 1M objects, destroy them, then create another 1M objects - that shouldn't grow the size in task manager.
--
Mats
Not really. A 3rd party tool may be necessary.
I don't know what to monitor, but Process Explorer has a lot of them.
Here is your bug. in function you use 'delete this' that means NOTHING is allowed to access the data in that object any more. You can't go object arrow anything - object is gone dead deleted destroyed erradicated eliminated erased... got it?Quote:
Code:object -> function(n);//call the object
if(object -> next != 0) object = object -> next;//if not last object, move to the next one
The fix:Code:dyn_obj *tempNext = object -> next;
object -> function(n);//call the object
if(tempNext != 0) object = tempNext ;//if not last object, move to the next one
thanks iMalc!
you solved my problem.
I didn't even think about that possibility...
This is still a pretty poor implementation, however.
I'd still suggest you try something like the example I made.
actually I'm looking for tutorials on STL right now...
Looking back over my post, I hope you realised I was being humourous... in a 'Johny 5' kind of way...
btw regardless of whether or not you actually use 'delete this', the remembering of the next pointer before you delete the object is a technique you learn once you've written a linked list that uses non-recursive deletion. It's kind of hard to forget.