Sign in to follow this  
3dmodelerguy

Issue with memory leak

Recommended Posts

I am string to build a string class as a learning example(so please no post about just use the std::string). this is the construct for a char pointer.
string::string(char* data_pointer)
{
	_size = (long)strlen(data_pointer);
	_pointer = new char[_size];
	memcpy(_pointer, data_pointer, _size);
};

now this work fine when i create the string with new but when i just do
ncommon::string test = ncommon::string("test");

I get the memory leak because the destructor is never called which deletes the memory created in the constructor. Is there a way to fix this? I am going to implement some pre allocated data of 20 or so but I know that is the string is bigger than 20 character I will get the error, it there any way to avoid this(i don't think so since the std::string has the same memory leak with a big enough string)?

Share this post


Link to post
Share on other sites
Quote:
Original post by 3dmodelerguy
I get the memory leak because the destructor is never called which deletes the memory created in the constructor. Is there a way to fix this?

The destructor is *always* called (unless there is an exception thrown from the constructor).

I have a feeling that your problem is caused by a missing (or incorrect) copy-constructor.

Share this post


Link to post
Share on other sites
Rule of Three

You need to define a copy constructor and assignment operator.


ncommon::string test = ncommon::string("test");



The above line first creates a temporary ncommon::string() object, then object is used to initialize test via copy-constructor. After all that happens the destructor is called for the temporary object thereby deleting its internal heap allocated members.

The problem is that the default copy and assignment operators perform "shallow copies", you want to perform a "deep copy".

Share this post


Link to post
Share on other sites
Quote:
Original post by 3dmodelerguy
does that mean the std::string also has an incorrect copy construct?

No, it means that std::string has a manually defined correct copy constructor.

Share this post


Link to post
Share on other sites
Quote:
Original post by Sneftel
Quote:
Original post by 3dmodelerguy
does that mean the std::string also has an incorrect copy construct?

No, it means that std::string has a manually defined correct copy constructor.


... and assignment operator, and destructor. Re: Rule of Three.

Share this post


Link to post
Share on other sites
Quote:
Original post by Sneftel
Quote:
Original post by 3dmodelerguy
does that mean the std::string also has an incorrect copy construct?

No, it means that std::string has a manually defined correct copy constructor.


but when I created a string will std:string that is say 60 character longs I get a memory leak

Share this post


Link to post
Share on other sites
Quote:
Original post by 3dmodelerguy
Quote:
Original post by Sneftel
Quote:
Original post by 3dmodelerguy
does that mean the std::string also has an incorrect copy construct?

No, it means that std::string has a manually defined correct copy constructor.


but when I created a string will std:string that is say 60 character longs I get a memory leak


I don't understand your question. In short: std::string works fine, your class is broken.

Anywhoo here is what is wrong with your code:

First, you aren't accounting for the null terminator in your original constructor.


string::string(char* data_pointer)
{
_size = (long)strlen(data_pointer) + 1; // Add one for the null terminator
_pointer = new char[_size](); // Make sure buffer is zereo'd
memcpy(_pointer, data_pointer, _size);
};







Here is the user defined copy constructor that you're lacking:

string::string(const string& rhs)
{
_size = (long)strlen(rhs._pointer) + 1; // Add one for the null terminator
_pointer = new char[_size](); // Make sure buffer is zereo'd
memcpy(_pointer, rhs._pointer, _size);
};







Here is the user defined assignment operator that you're lacking:

string::operator=(const string& rhs)
{
_size = (long)strlen(rhs._pointerr) + 1; // Add one for the null terminator
_pointer = new char[_size](); // Make sure buffer is zere'od
memcpy(_pointer, rhs._pointer, _size);
return *this;
};







And lastly you need a custom destructor:


string::~string()
{
if(_pointer)
{
delete [] _pointer;
_pointer = 0;
}
};





Share this post


Link to post
Share on other sites
Quote:
Original post by 3dmodelerguy
but when I created a string will std:string that is say 60 character longs I get a memory leak

Then it is due to a bug in your code. Getting this sort of thing right is trivial. Either the string hasn't been destroyed yet when you call your leak checker, or you're misreading the results.

Share this post


Link to post
Share on other sites
Quote:
Original post by fpsgamer
Here is the user defined assignment operator that you're lacking:
*** Source Snippet Removed ***

You forgot to free the previous memory addressed in _pointer ;)
string& string::operator=(const string& rhs)
{
char* oldPointer = _pointer;//don't do the delete immediately, in case new throws an exception. This way the internal state is preserved.
long size = (long)strlen(rhs._pointer) + 1; // Add one for the null terminator
_pointer = new char[size](); // Make sure buffer is zere'od
_size = size;
delete [] oldPointer;
memcpy(_pointer, rhs._pointer, size);
return *this;
};



Share this post


Link to post
Share on other sites
Quote:
Original post by Hodgman
Quote:
Original post by fpsgamer
Here is the user defined assignment operator that you're lacking:
*** Source Snippet Removed ***

You forgot to free the previous memory addressed in _pointer ;)
*** Source Snippet Removed ***


Good catch. Copy n' Paste will be the death of me :)

Share this post


Link to post
Share on other sites
I know this is for learning, but I usually leave string copying to strdup() function. It automatically allocates the right amount of memory to hold a string you want to copy, then copies string data and returns pointer to allocated buffer. The possible downside is that the pointer has to be released by free() function, the usage of which is advised against in the C++ realm.

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