- Forum
- General C++ Programming
- Looking for a memory leak. Not sure what
Looking for a memory leak. Not sure what I am doing anymore...
Sorry if this is short, I am being called to go somewhere. I will comeback and answerer any questions. I have been mulling over this code and I cannot figure out if I am making progress or not. It would be great if someone can look at it and offer any advise.
Please note the use of char * is going to be the end goal (for a class project) I am however ok with using std::sting to trouble shoot.
Thanks in advance.
https://gist.github.com/anonymous/611f6619eb7e03a393a4
Please note the use of char * is going to be the end goal (for a class project) I am however ok with using std::sting to trouble shoot.
Thanks in advance.
https://gist.github.com/anonymous/611f6619eb7e03a393a4
Things I noticed:
1) LinkedList.cpp line 16, you do not set pi_newItem->next to null in that if block. You only do that in the following code afterwards (which is never reached because of the return on line 18).
2) LinkedList.cpp line 40. If the head node is being deleted, your entire tree will completely screw up, because 'head' is never updated and you are left with a bad pointer. This could cause memory leaks among all sorts of other problems. But maybe this is not a problem because the head node doesn't seem to be a real node?
3) It's curious why a general container class like a LinkedList has any knowledge of a 'Player' class. That seems incredibly out of place.
4) HashTable and LinkedList classes manually manage memory, but do not have proper copy ctors and assignment operators. So if you attempt to copy/assign either one of them, you will have big problems (likely program crashes).
Remember the rule of 3: If you need any one of Destructor/Copy Ctor/Assignment operator, then you likely need all 3 of them.
5) If you can't use std::string, consider making your own string class instead of passing char*'s everywhere. It would greatly simplify your Player class.
6) Why does PlayerDB inherit from Player? A database is not a player -- that does not make any conceptual sense.
EDIT:
7) The Player class seems confused about whether or not it owns the name string. You do not delete[] the string in the dtor, which implies it does not assume ownership, yet it new[]s a string in setusername() which would require it to assume ownership.
8) LinkedList's dtor does not delete all nodes (memory leak). Line 130 of LinkedList.cpp:
9) Just FYI, delete or delete[] on a null pointer is harmless. So
1) LinkedList.cpp line 16, you do not set pi_newItem->next to null in that if block. You only do that in the following code afterwards (which is never reached because of the return on line 18).
2) LinkedList.cpp line 40. If the head node is being deleted, your entire tree will completely screw up, because 'head' is never updated and you are left with a bad pointer. This could cause memory leaks among all sorts of other problems. But maybe this is not a problem because the head node doesn't seem to be a real node?
3) It's curious why a general container class like a LinkedList has any knowledge of a 'Player' class. That seems incredibly out of place.
4) HashTable and LinkedList classes manually manage memory, but do not have proper copy ctors and assignment operators. So if you attempt to copy/assign either one of them, you will have big problems (likely program crashes).
Remember the rule of 3: If you need any one of Destructor/Copy Ctor/Assignment operator, then you likely need all 3 of them.
5) If you can't use std::string, consider making your own string class instead of passing char*'s everywhere. It would greatly simplify your Player class.
6) Why does PlayerDB inherit from Player? A database is not a player -- that does not make any conceptual sense.
EDIT:
7) The Player class seems confused about whether or not it owns the name string. You do not delete[] the string in the dtor, which implies it does not assume ownership, yet it new[]s a string in setusername() which would require it to assume ownership.
8) LinkedList's dtor does not delete all nodes (memory leak). Line 130 of LinkedList.cpp:
if (q) delete p; Why if(q)? Wouldn't you want to delete p regardless of whether or not q is null?9) Just FYI, delete or delete[] on a null pointer is harmless. So
if(foo) delete foo; is pointless... you can just say delete foo;. If foo is null, then nothing will happen -- it's perfectly safe. Relevant for HashTable's dtor.
Last edited on
Just checked your code with Deleaker and get bunch of memory leaks. Look at them:
http://snag.gy/fmtxY.jpg
etc.
Use smart pointers, don't call new and delete directly - this is the best advise.
http://snag.gy/fmtxY.jpg
etc.
Use smart pointers, don't call new and delete directly - this is the best advise.
Yet another instance of why
So, does that bring up compile errors for you?
Anyway, best practice is to get rid of
Maybe this will help a little :+)
using namespace std; is bad std::next exists in the std namespace.So, does that bring up compile errors for you?
Anyway, best practice is to get rid of
using namespace std;. put std:: before each std thing. You will see all the experts here always do that, so you should too :+)Maybe this will help a little :+)
Topic archived. No new replies allowed.