Archived

This topic is now archived and is closed to further replies.

doctorsixstring

String class bug

Recommended Posts

I have a bug in my app that I can''t seem to squash. I may be missing something obvious, so I would like to present my problem to the good folks at Gamedev. I have written my own string class called ''mstring'' (primarily as a learning experience, but I have added some custom funcitionality). Unfortunately, I am having a problem with the objects becoming corrupt when they are accessed with my "cstr()" function: SendMessage(m_hwnd_statusbar, SB_SETTEXT, (WPARAM)1, (LPARAM)m_strTest.cstr()); It seems that m_strTest''s destructor is being called by the SendMessage() function. This is obviously not intended, and definitely not desired. I have no idea why this would be happening. Does anyone have any thoughts/questions? Here is [most of] the code to my mstring class: I apologize for posting this much code, but due to the fact that I can''t pinpoint what exactly is going wrong, I feel that it is necessary. mstring.h
  
#include <math.h>
#include <stdio.h>

#ifndef MSTRING_H
#define MSTRING_H

class mstring
{
public:
	mstring();
	mstring(char* str);
	~mstring();

	int length();

	mstring operator=(char* str);

	char operator[](const int index) const;

	bool operator== (const mstring& str) const;
	bool operator!= (const mstring& str) const;

	operator void*();
	operator char*();
	operator const char*();

	char* cstr();

	void ReadFromFile(FILE* pFile);
	void WriteToFile(FILE* pFile);
	static void WriteToFile(FILE* pFile, char* pStr);

private:

	void set(char* str);
	void append(char* str);

	char* m_strBuffer;
	int m_nLength;				//# of characters stored in m_strBuffer, not counting the null terminator

										//  (set every time the buffer changes)

	mutable int m_charIndex;	//used for cycling through strs (defined as member to improve performance)

};

#endif
  
mstring.cpp
  
#include "mstring.h"

//

//

//

mstring::mstring()
{
	m_strBuffer = 0;
	m_nLength = 0;

	m_nPrecision = 5;
}


//

//

//

mstring::mstring(char* str)
{
	m_strBuffer = 0;
	m_nLength = 0;

	m_nPrecision = 5;

	set(str);
}


//

//

//

mstring::~mstring()
{
	if(m_strBuffer)
		delete [] m_strBuffer;

	m_strBuffer = 0;

	m_nLength = 0;
}


//

//

//

int mstring::length()
{
	return m_nLength;
}


//

//

//

mstring mstring::operator=(char* str)
{
//	MessageBox(0,m_strBuffer, "mstring operator=() called",0);

	set(str);

	return m_strBuffer;
}


//

//

//

char mstring::operator[](const int index) const
{
//	MessageBox(0,m_strBuffer, "mstring operator[]() called",0);


	if(m_strBuffer && index>=0 && index<m_nLength)
	{
		MessageBox(0,&m_strBuffer[index], "Retrieving mstring value (operator[])==",0);
		return m_strBuffer[index];
	}
	else
		return 0;
}


//

//

//

bool mstring::operator==(const mstring& str) const
{
//	MessageBox(0,m_strBuffer, "mstring operator==(const mstring& str) called",0);


	m_charIndex = 0;

	while(m_strBuffer[m_charIndex] == str[m_charIndex])
	{
		if(m_strBuffer[m_charIndex] == ''\0'' && str[m_charIndex] == ''\0'')
			return true;

		++m_charIndex;
	}

	return false;
}


//

//

//

bool mstring::operator!=(const mstring& str) const
{
//	MessageBox(0,m_strBuffer, "mstring operator!=(const mstring& str) called",0);

	m_charIndex = 0;

	while(m_strBuffer[m_charIndex] == str[m_charIndex])
	{
		if(m_strBuffer[m_charIndex] == ''\0'' && str[m_charIndex] == ''\0'')
			return false;

		++m_charIndex;
	}

	return true;
}


//

//

//

void mstring::set(char* str)
{
	//this will set the string to "0" if the user passes NULL as a parameter.  Perhaps it should just create an

	//	empty string?

	if(!str)
	{
		m_nLength = 1;
		m_strBuffer = new char[m_nLength+1];
		m_strBuffer[0] = ''0'';
		m_strBuffer[1] = 0;
		return;
	}

	//count the length of ''str''

	m_charIndex = 0;

	while(str[m_charIndex] != ''\0'')
		++m_charIndex;

	m_nLength = m_charIndex;

	//temporarily save the old string so that we can delete when you are no longer using it

	char* pOldBuffer = m_strBuffer;

	//allocate a new mstring buffer

	m_strBuffer = new char[m_nLength+1];

	//fill in the mstring buffer with the characters from ''str''

	while(m_charIndex>=0)
	{
		m_strBuffer[m_charIndex] = str[m_charIndex];
		--m_charIndex;
	}

	//clear the old buffer (if it exists)

	delete [] pOldBuffer;
}


//

//

//

void mstring::append(char* str)
{
/*	char debugstr[256];
	sprintf(debugstr, "about to append ''%s'' to ''%s''", str, m_strBuffer);
	MessageBox(0,debugstr,0,0);*/

	//count the length of ''str''

	m_charIndex = 0;

	while(str[m_charIndex] != ''\0'')
		++m_charIndex;

	char* pOldBuffer = m_strBuffer;

	//allocate a new string buffer

	m_strBuffer = new char[m_nLength+m_charIndex+1];

	m_charIndex = m_nLength+m_charIndex;

	//fill in the mstring buffer with the old string + the characters from ''str''

	while(m_charIndex>=m_nLength)
	{
		m_strBuffer[m_charIndex] = str[m_charIndex-m_nLength];
		--m_charIndex;
	}

	while(m_charIndex>=0)
	{
		m_strBuffer[m_charIndex] = pOldBuffer[m_charIndex];
		--m_charIndex;
	}

	delete [] pOldBuffer;
}

mstring::operator void*()
{
	MessageBox(0,m_strBuffer, "Retrieving mstring value (operator void*)==",0);
	return (void*)m_strBuffer;
}

mstring::operator char*()
{	
//	MessageBox(0,m_strBuffer, "Retrieving mstring value (operator char*)==",0);

	return m_strBuffer;
}

mstring::operator const char*()
{	
	return (const char*)m_strBuffer;
}

char* mstring::cstr()
{	
	return m_strBuffer;
}

//

//	This function will read an mstring object from the specified, previously-opened file.  mstring objects

//		are written to a file with an integer preceding the actual text.  This integer specifies the length

//		of the string in bytes (i.e. # of characters in the string, not including a null-terminator).

//

void mstring::ReadFromFile(FILE* pFile)
{
	if(pFile)
	{
		int nStrLength;	

		//get the string length of the image''s path	

		fread(&nStrLength, sizeof(int), 1, pFile);

		char* strText = new char[nStrLength+1];		//allocate space for the string + a null terminator

		fread(strText, sizeof(char), nStrLength, pFile);
		strText[nStrLength] = 0;

		set(strText);

		delete strText;
	}
}


//

//

//

void mstring::WriteToFile(FILE* pFile)
{
	//if the given file pointer AND the text buffer are both valid, we can go ahead and write the

	//	string to the file

	if(pFile && m_strBuffer)
	{
		//write the length of the string (w/o a null-terminator) to the file

		fwrite(&m_nLength, 1, sizeof(int), pFile);

		//write the characters

		fwrite((void*)m_strBuffer, 1, m_nLength, pFile);
	}
}

//

//

//

void mstring::WriteToFile(FILE* pFile, char* pStr)
{
	if(pFile && pStr)
	{
		//count the length of ''str''

		int nLength = 0;

		while(pStr[nLength] != ''\0'')
			++nLength;

		fwrite(&nLength, 1, sizeof(int), pFile);
		fwrite((void*)pStr, 1, nLength, pFile);
	}
}
  

Share this post


Link to post
Share on other sites
Other than the fact that your append() method doesn''t update m_nLength, I don''t see any thing that doesn''t work. (I didn''t look at the file i/o functions.) The class worked and didn''t appear to corrupt anything.

Are you sure you don''t have some rogue code somewhere else that''s stomping on something?

Share this post


Link to post
Share on other sites
It is finished! The most diabolical bug ever has been squashed! It took me many hours, but here is what was going wrong:

When I pass an mstring object to a function, that function creates a local copy of the passed object. I did not have a copy constructor, so the compiler did the best it could and simply created another mstring object with the same values (including the pointer to the string buffer).

Unfortunately, this is not good at all. What basically happens is I had two mstring objects, both of which pointed to the same string buffer . When the function ends and the local variables are deleted, the string buffer is also deleted (by the local mstring variable's destructor). Voila! Instant data corruption.

So anyways, I implemented a copy constructor that creates a seperate string buffer for the local variable, and I lived happily ever after.

-Mike

[edited by - doctorsixstring on May 5, 2003 1:02:53 PM]

Share this post


Link to post
Share on other sites
A couple things:

1) whenever you have a operator= it is usually advisable to have a copy constructor. Here's how I implement things:


class mstring {
void copy(const mstring& o) {
// do all your deep copying here
m_length = o.m_length;
// etc
}
public:
mstring( const mstring& o ) {
copy(o);
}
mstring& operator=(const mstring& o) {
copy(o);
return *this;
}
// ...
};


In this way you localize your copying code.

2) Are you sure you want to return a mstring object from your operator=? This would probably be very inefficient, especially in such case like:

s1 = s2 = s3 = "Hello"; // how many temporaries do you have?

3) Your length() method should be const.

4) If you redefine your operator[] method to this:

char& operator[](const int index);

You could write things like this:

mstring s1 = "Hello";
s1[4] = 'z';

You have to be careful of going out of the bounds of the array though (as with regular char buffers and strings).

5) Your operator!= could be defined in terms of your operator== to ensure consistency:

bool operator!=(const mstring& o) { return !(*this == o); }

6) Not sure why you want a operator void*(), but if you do, at least use C++-style casts

Regards,
Jeff

Edit: Added points 3 through 6 after looking at the code more.


[edited by - rypyr on May 5, 2003 1:42:40 PM]

Share this post


Link to post
Share on other sites