Code crashes in main after successful char * handling

[dannz89]

Hello,

I'm sure this is some complete beginner's 'gotcha' that I've forgotten all about but if anyone can give me a clue as to why my pointer seems to be going astray....?

I'm new to C++, well, very old to C++ actually but have been programming JAVA for years and now have cause to dust off my C++ and practise a bit.

To this end, I am writing a String class (yes, I know one already exists but it's a good enough exercise to start).

I am at the point where I'm overloading the += and = operator only my += operator overload, although it works in the class implementation, right through to the end of the last method call, when the program gets back into main, it crashes.

The line after which it crashes is:

s2 += "YYY";

however the debugging output from all the cout calls in String.cpp suggest that everything has worked fine.

I'm using cygwin on Windows vista. My cygwin installation is up to date and my GCC reports the following version:
g++ (GCC) 3.4.4 (cygming special, gdc 0.12, using dmd 0.125)
Copyright (C) 2004 Free Software Foundation, Inc.
This is free software; see the source for copying conditions. There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

I'm using a makefile although it's pretty make-shift (no pun intended!). Basically all it does is

g++ -c test.cpp String.cpp
g++ -o test.exe test.o String.o

I have 3 files, test.cpp (with the main function), String.h and String.cpp. I'll attempt to put the code in here for those three respectively:

test.cpp

#include "String.h"
#include <iostream>

using namespace std;

int main(int argc, char **argv)
{
	cout << "Started." << endl;
	String s("Test");
	cout << "Set up string s." << endl;
	String s2(s);
	cout << "Set up string s2." << endl;
	cout << "String s is: [" << s.getString() << "]" << endl;
	cout << "String s2 is: [" << s2.getString() << "]" << endl;
	s.setString("Second value");
	cout << "String is: [" << s.getString() << "] after second setting." << endl;
	cout << "String s2 is: [" << s2.getString() << "] after second setting." << endl;
	s2 += "YYY";
	cout << "Set s2 again." << endl;
	cout << "String s2 is: [" << s2.getString() << "]." << endl;
	cout << "Finished." << endl;
	return 0;
}

String.h

class String
{
	public: 
		String();
		String(const char *);
		String(const String &);
		~String();
		void setString(const char *);
		void setString(const String &);
		const char * getString() const;
		String operator = (const String &);
		String operator += (const String &);
		String operator += (const char *);	

	private:
		void addOrConcatenate(const char *);
		char *string;
};

String.cpp

#include <stdlib.h>
#include <string.h>
#include <iostream>
#include "String.h"

using namespace std;

String::String()
{
	this->string = NULL;
	this->setString("");
}

String::String(const char *s)
{
	this->string = NULL;
	this->setString(s);
}

String::String(const String &s)
{
	this->setString(s);
	cout << "End Copy Constructor..." << endl;
}

String::~String()
{
	if(this->string!=NULL)
	{
		delete[]this->string;
		this->string = NULL;
	}
}

void String::setString(const String &s)
{
	this->setString(s.getString());
}

void String::setString(const char *s)
{
	cout << "10" << endl;
	if (this->string!=NULL)
	{
		cout << "11" << endl;
		delete[]this->string;
		this->string=NULL;
		cout << "12" << endl;
	}
	cout << "13" << endl;
	this->string = new char[strlen(s)+1];
	cout << "14" << endl;
	strcpy(this->string,s);
	cout << "string is: [" << this->getString() << "]." << endl;
	cout << "15" << endl;
}

const char * String::getString() const
{
	cout << "5" << endl;
	return (const char *)this->string;
}

String String::operator = (const String &s)
{
	this->setString(s.getString());
}

String String::operator += (const String &s)
{
	cout << "6" << endl;
	this->addOrConcatenate(s.getString());
	cout << "7" << endl;
}

String String::operator += (const char *s)
{
	cout << "8" << endl;
	this->addOrConcatenate(s);
	cout << "9" << endl;
}

void String::addOrConcatenate(const char *s)
{
	char *tempString = NULL;

	cout << "1" << endl;

	if(this->string!=NULL)
	{
		cout << "2" << endl;
		tempString = new char[strlen(this->string)+strlen(s)+1];

		size_t len_string = strlen(this->string);
		
		size_t i=0;

		for(i = 0; i<len_string; i++)
		{
			printf("Copying character [%c]\n",this->string[i]);
			tempString[i] = this->string[i];
		}

		cout << "i is [" << i << "]." << endl;
		tempString[i] = '\0';

		strcat(tempString,s);
		cout << "tempString is: [" << tempString << "]." << endl;
	}
	cout << "3" << endl;
	cout << "tempString outside if() is: [" << tempString << "]." << endl;
	this->setString(tempString);
	cout << "4" << endl;
}

[kvprajapati]

Problem with += operator. Use void instead of String to prevent a copy of string object.

void operator += (const char *);

Read - http://www.parashift.com/c++-faq-lite/operator-overloading.html

[Ancient Dragon]

Observation: string.h is a standard C header file -- rename yours to something else to avoid filename clashes. In MS-Windows file names are not case sensitive like they are in *nix, so String.h is the same file as string.h, which is clearly not what you want because your program is using some of the functions in the standard string.h header file.

Also I would rename that String class to something else to avoid confusion with the string class in STL library.

String::String(const String &s)
{
	this->setString(s);
	cout << "End Copy Constructor..." << endl;
}

One problem is with the above code. You forgot to set this->stirng = NULL; before calling setString().

String& operator = (const String &);
		String& operator += (const String &);
		String& operator += (const char *);

Make the above three functions return a reference to the String, not a new copy of the String. Then at the end of each of those functions just add return *this; Your class test program worked without fail after making the above changes.

[dannz89]

Observation: string.h is a standard C header file -- rename yours to something else to avoid filename clashes. In MS-Windows file names are not case sensitive like they are in *nix, so String.h is the same file as string.h, which is clearly not what you want because your program is using some of the functions in the standard string.h header file.

Also I would rename that String class to something else to avoid confusion with the string class in STL library.

String::String(const String &s)
{
	this->setString(s);
	cout << "End Copy Constructor..." << endl;
}

One problem is with the above code. You forgot to set this->stirng = NULL; before calling setString().

String& operator = (const String &);
		String& operator += (const String &);
		String& operator += (const char *);

Make the above three functions return a reference to the String, not a new copy of the String. Then at the end of each of those functions just add return *this; Your class test program worked without fail after making the above changes.

Hi, thanks for that. I found the trick of changing the method signatures to:

String & operator += (const char *);

and also a new one

String & operator += (const String &);

on another site. Similarly, the return *this; trick. I'd also tidied up the not setting this->string to NULL in the copy constructor.

Re: the 'case sensitivity' of the filenames, I did wonder about it and double-checked. I'm working in cygwin which basically turns a DOS box in a *NIX or *NIX-like shell. My experimentation has shown that it honours case sensitivity in file names same as *NIX. It's a very good point though. Good idea re: changing them to avoid confusion also.

The trouble is I went out for a walk before posting my findings. I found this page really useful for an overview of overloading operators.

http://www.cs.caltech.edu/courses/cs11/material/cpp/donnie/cpp-ops.html

You already kind of have to know what you're doing at least a bit for it to be meaningful but as long as you do, it's got some good reminders in it.

Generally, your reply is excellent because it tells me I did the right thing(s)!

Many thanks.