Program invokes copy constructor when I thought it should invoke assignment operator=

[BaltaX]

In the following program file (which tests a homegrown string class called String, see code below), the row

str2 = str3 + str1; //Denna kallar på copy constructorn - VARFÖR!!!!

first invokes the operator+ function of String.cpp to concatenate str3 and str1, but then, instead of invoking the assignment operator= , the copy constructor is called. I do not understand why, since str2 has already been defined. Also, after the copy constructor has been called, the execution goes to the destructor and ends in an error message:

Windows has triggered a breakpoint in Steg2_2.exe.

This may be due to a corruption of the heap, which indicates a bug in Steg2_2.exe or any of the DLLs it has loaded.

Using command str2 = str3; instead of str2 = str3 + str1; works as expected, with invocation of assignment operator and no error message. I am completely stuck on this.

Program file:

#include <iostream>
#include "String.h"
using std::cout;
using std::cin;
using std::endl;

class Application
{
public:
	int run();
};

int Application::run()
{
	String str1("Hello ");
	String str2(str1);
	cout << str1 << str2 << endl;	// Ska skriva ut "Hello Hello "

	str2 = "my world!";
	cout << str1 << str2 << endl;	// Ska skriva ut "Hello my world!"

	String str3;
	cout << "Skriv ett namn: ";
	cin >> str3;
	// ( Användaren skriver t.ex. "Pluto" )
	
	str2 = str3 + str1; //Denna kallar på copy constructorn - VARFÖR!!!!
	
	cout << str2 << endl;		// Ska skriva ut "Hello Pluto"
	//str2 = "Goodbye " + str3;
	//cout << str2 << endl;		// Ska skriva ut "Goodbye Pluto"
	//cout << str1 << "och " << "Goodbye " 
	//		<< (str1 == "Goodbye " ? "är " : "är inte ") << "samma ord!\n";
	//							// Ska skriva ut "Hello och Goodbye är inte samma ord!"
	cin.get();

	return 0;
}

int main()
{
	Application myApp;
	return myApp.run(); 
}

String.h:

#ifndef STRING_H
#define STRING_H
#include <iostream>


class String
{
	friend std::ostream& operator << (std::ostream& os, const String& Str);
	friend std::istream& operator >> (std::istream& is, String& Str);
private:
	char* _strPtr;
	int _strLen;

public:
	//Constructors
	String();
	String(char* p);
	String(const String& Str);

	//Assignment operator
	String& operator = (const String& Str);

	//Concatenate String + String
	String operator + (const String& Str);

	//Destructor
	~String();

};

#endif 

String.cpp:

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

using namespace std;

//Default constructor
String::String()
	{
		_strLen = 1;
		_strPtr = new char[_strLen];
		memcpy(_strPtr,"",_strLen);
	}

//Constructor for C string literal
String::String(char* p)
	{
		_strLen = strlen(p)+1;
		_strPtr = new char[_strLen];
		memcpy(_strPtr,p,_strLen);
	}

//Copy constructor
String::String(const String& Str)
	{
		_strLen = Str._strLen;
		_strPtr = new char[_strLen];
		memcpy(_strPtr,Str._strPtr,_strLen);
	}

//Assignment operator =
String& String::operator = (const String& Str)
{
	char* temp = new char[Str._strLen];
	_strLen = Str._strLen;
	memcpy(temp,Str._strPtr,_strLen);
	delete[] _strPtr;
	_strPtr = temp;
	
	return *this;
}

//Global function
//Operator << (for cout << String commands)
ostream& operator << (ostream& os, const String& str)
{
    short i;
    for (i=0; i < str._strLen-1; i ++)
        os << str._strPtr[i];

    return os;
}

std::istream& operator >> (std::istream& is, String& Str)
{
    const short BUFLEN = 256;
	char Buf[BUFLEN];
	memset(Buf,0,BUFLEN);
	if(is.peek() == '\n')
		is.ignore();
	is.getline(Buf,BUFLEN,'\n');
	Str = Buf; //Buf is a char*, so constructor String(char *) is called here, which creates a temporary string
				//Then operator= is called to copy that string to Str
    return is;
}

//Concatenate String + String
	String String::operator+ (const String& Str)
	{
		//Str is b in a+b; "this" is a
		
		String temp; //temp="", has called Default Constructor
		strcpy(temp._strPtr, _strPtr);
		temp._strLen = _strLen + Str._strLen-1;
		strcat(temp._strPtr, Str._strPtr);
		return temp;
	}

//Destructor
String::~String()
{
	delete []_strPtr;
	
}

[Disch]

It's doing all 3

1) operater + to concatenate the strings
2) copy ctor to return the object from the + operator
3) operator = to assign the returned object to a new object


Note that this really shouldn't matter. If your operators and ctors are written properly, it shouldn't matter which are called and when.

From looking at your code, right away I see that your + operator is broken:

	String String::operator+ (const String& Str)
	{
		//Str is b in a+b; "this" is a
		
		String temp; // temp only has 1 char allocated
		strcpy(temp._strPtr, _strPtr);  // this will overflow unless _strPtr is an empty string!!!
		temp._strLen = _strLen + Str._strLen-1;
		strcat(temp._strPtr, Str._strPtr);  // more overflow!!!
		return temp;
	}

This overflow will cause heap corruption and will do nasty things to your program. If you're lucky it will just crash.

[andywestken]

You code is pretty cool, but there are a few issues you could deal with. For example, protection against NULL char* strings.

Would you like some more general feedback? Or do you just want to on the single issue?

[BaltaX]

@Disch, thanks a lot, you spotted the problem; now it works! New code for operator+ that made it work:

//Concatenate String + String
	String String::operator+ (const String& Str)
	{
		String temp; 
		temp._strLen = _strLen + Str._strLen-1;
		temp._strPtr = new char[temp._strLen]; //dynamic memory allocation
		strcpy(temp._strPtr, _strPtr); //Put whats in str1 into temp._strPtr
		strcat(temp._strPtr, Str._strPtr); //Add whats in str3 to temp._strPtr
		return temp;
	}

@andywestken, I would very much like general feedback as well, thanks

[andywestken]

Well, the code is good; it's just a few loose ends I'm talking about.

1. could use

_strPtr = new char[_strLen]();

to avoid the call to strcpy in the default constructor -- the () zeros the buffer (this is more a style preference)

(I would also use

char Buf[BUFLEN = {0};

to avoid memset, but am not 100% this is ANSI)

2. should use const char* -- rather than just char* -- for the C-string constructor's param

3. should probably protect strlen() against a null pointer

(as strlen will die with a null pointer; this depends on how defensive you want to be)

4. _strLen = strlen(p)+1; suggests your member is really the buffer length, rather than the string length

Same with maths in operator+

5. I would move the for loop variable inside the loop -- for(short i=0; ... -- unless you need to get at it after the end of the for scope

6. I could use an unsigned type for the buffer length

a -ve size for a buffer makes no sense, so why allow it; stl uses size_t (usually a typedef of unsigned int)

7 operator<< could just write the char* out (is there a reason you're outputting a null terminated string char by char?)

(could guard against null here; ostream doesn't do so)

That's about it!