Jump to content
  • Advertisement
Sign in to follow this  
3dmodelerguy

Issue with memory leak

This topic is 3713 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

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
Advertisement
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
Sign in to follow this  

  • Advertisement
×

Important Information

By using GameDev.net, you agree to our community Guidelines, Terms of Use, and Privacy Policy.

We are the game development community.

Whether you are an indie, hobbyist, AAA developer, or just trying to learn, GameDev.net is the place for you to learn, share, and connect with the games industry. Learn more About Us or sign up!

Sign me up!