Cleaning up null terminated strings

Started by
6 comments, last by smr 19 years, 3 months ago
After much searching I've found the source of a memory leak in my program. I have global functions for writing debug data to a null terminated string:

static void AppendDebugText(const char* text){

	if (DEBUGGING){
		char* tmp = new char[strlen(text) + strlen(g_DebugText) + 1];
		
		for (unsigned int i = 0; i < strlen(g_DebugText); i++) tmp = g_DebugText;

		for (unsigned int j = 0; j < strlen(text) + 1; j++) tmp[i++] = text[j];

		delete [] g_DebugText;
		g_DebugText = new char[strlen(tmp) + 1];
		strcpy(g_DebugText, tmp);
		delete [] tmp;
	}
}

When I set DEBUGGING false there is no memory leak. None of strings passed are declared with the new operator, all on the stack. There are a couple of other functions similar to this one, all following the same layout. What am I doing wrong? Any help much appreciated.
Dave.
Advertisement
You never delete[] the last allocated instance of g_DebugText. The solution? std::string:
static std::string g_DebugText;static void AppendDebugString(const std::string& text){	if (DEBUGGING)	{		g_DebugText += text;	}}


Enigma
Ah, I do delete the last instance elsewhere. The problem is there is a memory leak whilst the program is running. ctrl-alt-del'ing and checking how much memory the process is using shows the memory usage constantly increasing whilst the program is running (AppendDebugText as well as some others are called every time step).

I'd rather not use a string, it would mean a lot of hassle in the rest of the code and I would be no the wiser why this problem is happening. But, then, if I cant find a solution I'll have to.
Dave.
Well, if you delete[] the last instance then there is no memory leak in that code snippet, although it's not exception safe and in fact not valid C++ (The C++ defines the scope of a variable declared within a for loop statement to be the same as the scope of the for loop. You must be using either Visual C++ 6 or a later version of Visual C++ without enforcing standard for loop scoping).

Task manager is not a reliable way to detect memory leaks and your 'leak' may not actually exist. There is a set of functions in Visual C++ to monitor memory usage (crtdbg?) - try using that instead.

That said, std::string really is what you should be using for textual data in C++ and converting existing code to use it should not be too difficult.

Enigma
Way too much work going on.
static void AppendDebugText(const char* text){  if (DEBUGGING){    char* tmp = new char[strlen(text) + strlen(g_DebugText) + 1];    strcpy(tmp, g_DebugText);    strcat(tmp, text);    delete [] g_DebugText;    g_DebugText = tmp;  }}
Quote:Original post by Oluseyi
Way too much work going on.
*** Source Snippet Removed ***
Exactly what I was going to say.
It was pointless to allocate memory the same size as tmp, then copy the data into it from tmp, then delete tmp. Why not just do g_DebugText = tmp; and forget allocating g_DebugText and deleting tmp.

btw there is no leak, but you might think there is because tmp gets bigger by strlen(text)+1 each call to AppendDebugText.

It's gonna get slower and slower too this way as after a while you're going to be copying a huge amount of memory each time. You should preallocate g_DebugText to say 64K beforehand rather than constantly creating a bigger buffer each time. That will be much faster since you can merely append and you're done. Check that you don't exceed the buffer though.

Although, why not write it to file instead?
What use is keeping all of the debug info around in memory anyway?
"In order to understand recursion, you must first understand recursion."
My website dedicated to sorting algorithms
edit: realised I'd said what everyone else said: use std::string.
Simple ...

freopen("errors.txt", "w", stderr);...fprintf(stderr, "Heres a number: %d\n", 10);...fclose(stderr);

This topic is closed to new replies.

Advertisement