Operator overloading problem with Ms Visual studio

[wronski123]

Dear forum Members,
I have recently observed a very strange error when I tried to run a small test program under the Microsoft visual studio 2010 ultimate. I basically define two classes than I overload the “plus” operator. The curious thing is that with icc under Linux I can run the same code without any problems.
I would be very grateful if some of the experts here can help me by pointing out what is wrong with the code.

Thank you in advance.
Al

Here is the code

#include <iostream>
#include <cstdlib>
#include <string.h>

using namespace std;

class test {

public:

	int x,y;
	char *point;

	test() {
		point = new char [10];
		strcpy(point, "Test1");
		x = 0;
		y=0;
	}

	test(int a,int b) {
		point = new char [10];
		strcpy(point, "Test2");
		x=a;
		y=b;
	}

	test (const test& t) {
		x = t.x;
		y = t.y;
		point = new char [10];
		strncpy(point, t.point, 10);
	}

	~test() {
		cout<<"Destructor call " << x <<endl;

		if (point)
			delete[] point;
		point = NULL;

	}

	test operator+(test t1) {
	test temp;
	temp.x=x+t1.x;
	temp.y=y+t1.y;
	strncpy(temp.point,t1.point,10);
	return temp;
}

	
};




int main () {

#ifdef this_will_fail
	test X1(1,1),X2(5,5),X3;
	X3 = X1 + X2;

	
#elif  this_will_be_ok
   test X1(1,1),X2(5,5);
    test X3 = X1 + X2;

	
#endif

   


	cout << "post addition" << endl;
	X3.x = 7;
	return 0;
}

[Disch]

You are using the compiler-provided assignment operator which will not work because it only does a shallow copy of the 'point' pointer.

What's happening is something like this:

-) X3's default constructor new[]'s a buffer for its point (I'll call this buffer "bufA")
-) In your + operator, temp's constructor new[]'s another buffer (call it "bufB")
-) + operator returns
-) 'X3' gets assigned to 'temp'. This results in the bufA buffer being lost (memory leak), and now X3 points to bufB instead of bufA
-) 'temp' is destructed, resulting in bufB being delete[]'d
-) 'X3' now points to bad memory, because bufB no longer exists, but it still points to it.
-) at the end of main, X3 goes out of scope and tries to delete[] bufB again (even though it's already been deleted), likely resulting in a program crash.


Don't allocate memory unless you have to. Here you clearly don't have to. It's just greatly complicating your program and providing you zero benefit.


Solutions (in the order in which I'd recommend them):

1) Use a string instead of a char*.
2) Use a char[10] instead of a char* (ie: fixed size array instead of being dynamically sized)
3) Overload the = operator so it does a deep copy of the 'point' member similar to how your copy constructor is doing it.



Note that if you choose solutions 1 or 2... not only do you not have to write a = operator... but you also can get rid of your copy constructor and destructor because you won't need them anymore.



EDIT:

Also, in the future, it really helps if you tell us what the error is. Just saying "I get a weird error when I run it" is not very descriptive.

[wronski123]

Thank you for your replay,
I really apologize for not posting the error message, it was on a windows pop up and I didn’t include a print screen. For the future I would have that in mind. The error is contained in the fact that the destructor fails to free the memory. I now understand what the problem is.
I would like to ask you another question, if I may. Usually in C++ books they say that if a copy constructor is included ,there is no problem returning a class from a function (pass a class to a function). Why here the copy constructor fails to tackle this. I am a bit confused.
Last but not least I would like to thank you for your replay.
Al

[Athar]

Why here the copy constructor fails to tackle this.

It doesn't, it fails at the assignment in line 63, as you provided no assignment operator.
See also: http://en.wikipedia.org/wiki/Rule_of_three_%28C%2B%2B_programming%29

[Disch]

The error is contained in the fact that the destructor fails to free the memory.

That is incorrect.

The destructor is freeing the memory properly. There is no problem there.

The problem is the lack of assignment operator. The default assignment operator does a shallow copy of the pointer, which is destroying your object. You need to do a deep copy. Therefore you need to implement your own assignment operator. That's what the problem is.

[wronski123]

Hi
First of all I would like to thank Athar and Dish for their comments. Well what should I say, use FORTRAN and not C++. In FORTRAN everything is so elegant and beautiful, no pointer no memory leaks magnificent, and it runs much faster :) :) :) . Also the operator overloading and memory allocation is a piece of cake.

Here is how I understood the things, I hope it is correct:
The source of my problem was the fact that I was lacking a proper assignment operator. I have a copy constructor, which takes care when I pass the objects as arguments. However in my original code I lack a proper assignment operator. If I set the object equal to the temporary object returned by the overloaded (+) I get a memory leak, due the fact that the copy constructor is called only in the cases when we have initialization, but not in the simple equal case. This is the reason why in my original code the version

// this fails always due to the lack of proper assignment operator,
// i.e. only shallow copy is made when we have simple equal.  
	test X1(1,1),X2(5,5),X3;
	X3 = X1 + X2;

// this works because the copy constructor is called in the assignment
// operation

    test X1(1,1),X2(5,5);
    test X3 = X1 + X2;

After the very useful comments made by Athar and Dish, I included an overloaded assignment operator.I return a reference with my assignment operator because this was a recommended in a book, I do not entirely understand why this is needed. The code works fine without it. Now it is possible to run the code without errors. The updated version of the code is included below. Just out of pure curiosity is it possible to get along only with the copy constructor or I must always have the overloaded (=) ?

#include <iostream>
#include <cstdlib>
#include <string.h>

using namespace std;

class test {

public:

	int x,y;
	char *point;

	test() {
		point = new char [10];
		strcpy(point, "Test1");
		x = 0;
		y=0;
	}

	test(int a,int b) {
		point = new char [10];
		strcpy(point, "Test2");
		x=a;
		y=b;
	}

	test (const test& t) {
		x = t.x;
		y = t.y;
		point = new char [10];
		strncpy(point, t.point, 10);
	}

	~test() {
		cout<<"Destructor call " << x <<endl;

		if (point)
			delete[] point;
		point = NULL;

	}

	test operator+(test t1) {
	test temp;
	temp.x=x+t1.x;
	temp.y=y+t1.y;
	strncpy(temp.point,t1.point,10);
	return temp;
}

	test & operator=(test t1) {
		x=t1.x;
		x=t1.y;
		point = new char  [10];
		if (!point) {
			cout << "Allocation error" << endl;
			exit(0) ;
		}
		else{
			strncpy(point,t1.point,10);
		}
		return *this ;

	}
    


	void show(){cout<<point<<endl;}
};




int main () {


	test X1(1,1),X2(5,5),X3;

	X1.show();
	X2.show();
	X3.show();
	
	

    X3 = X1 + X2;
    
	

	X1.show();
	X2.show();
	X3.show();


	return 0;
}

[Disch]

use FORTRAN and not C++. In FORTRAN everything is so elegant and beautiful, no pointer no memory leaks magnificent, and it runs much faster :) :) :)

C++ is elegant too, if you do it right. You are just doing things in an overly complicated manner. I'm sure you could also do things in FORTRAN which make the code more complicated than it needs to be.

Also, high level languages don't get much faster than C and C++.

Just out of pure curiosity is it possible to get along only with the copy constructor or I must always have the overloaded (=) ?

The so called "rule of 3" says that there are 3 key areas of a class:
1) Copy constructor
2) Assignment operator
3) Destructor

If you find yourself having to write one of these functions, you likely have to write all 3.

The updated version of the code is included below

You are still leaking memory.

your default constructor new's a buffer.
The assignment operator new's another buffer without deleting the old one.

Since the buffer size is not changing, you don't need to reallocate with new in the assignment operator.




...

but again... you don't need (and shouldn't be using) new at all. It's just making your code way more complicated than it needs to be.


The simpler way to do this:

class test
{
public:

	int x,y;
	string point;  // <- make it a string

	test() {
		point = "Test1"; // then you can just assign it normally
		x = 0;
		y=0;
	}

	test(int a,int b) {
		point = "Test2"; // ditto
		x=a;
		y=b;
	}

    // no need to write copy ctor or destructor anymore
    //   strings are automatically deep copied, so no worry about memory leaks
    //   or pointer management or any of that.

	test operator+(test t1) {
	test temp;
	temp.x=x+t1.x;
	temp.y=y+t1.y;
	temp.point = t1.point; // normal assignment instead of strncpy
	return temp;
    }

    // no need for assignment operator, either.  For same reasons as above

    void show(){cout<<point<<endl;}
};

EDIT:


Or if you don't want to use strings for some weird reason... another simpler alternative:

class test
{
public:

	int x,y;
	char point[10];  // <- make it an array (no need for dynamic size)

	test() {
		strcpy(point,"Test1"); // can strcpy it
		x = 0;
		y=0;
	}

	test(int a,int b) {
		strcpy(point,"Test2"); // can strcpy it
		x=a;
		y=b;
	}

    // no need to write copy ctor or destructor anymore
    //   arrays are automatically deep copied, so like strings, there's no worry

	test operator+(test t1) {
	test temp;
	temp.x=x+t1.x;
	temp.y=y+t1.y;
	strcpy(temp.point,t1.point); // <-
	return temp;
    }

    // no need for assignment operator, either.  For same reasons as above

	void show(){cout<<point<<endl;}
};

[wronski123]

Dear Dish,

Well my example was, I have to admit, doing things in overly complicated way, but I posted it because I wanted to learn what is going wrong. I would like to go into the origins of the problem. I fully understand what you told me, i.e. if I use “string point” or even “char point[10]” I can resolve all my problems.

I am quite disappointed that my overloaded assignment operator leaks memory once again. Can I resolve this problem by first deleting the old “this.point” and reassigning it once immediately after. This is a question out of purely didactic origin.

test  operator=(test t1) {
		x=t1.x;
		x=t1.y;
		delete [] point;

		

		point = new char  [10];
		if (!point) {
			cout << "Allocation error" << endl;
			exit(0) ;
		}
		else{
			strncpy(point,t1.point,10);
		    	
		}
		return *this ;

	}

[Disch]

Yes that is one way to do it.

The other way is to simply not reallocate:

test operator=(test t1) {
		x=t1.x;
		y=t1.y;  // <- I assume you meant y, not x.  typo in your original code
		strncpy(point,t1.point,10); // <-no need to reallocate point, just use the existing buffer.
		return *this ;
	}

After all, what's the point of freeing a buffer only to allocate a new one of the same size?

With that, your class works and will not leak memory.


A few other minor things unrelated to your original question:

1) Your = and + operators should be taking const references (like your copy constructor):
test operator+(const test& t1) {

2) Your = operator (but not your + operator) should be returning a reference, not a copy:
test& operator=(const test& t1) {

3) Your + operator (but not your = operator) should be a const function, since it does not modify 'this':
test operator+(const test& t1) const {

4) new will never return a null pointer. So there's no need to check for that. In the case of an allocation failure, it will throw an exception.

[wronski123]

Dear Dish,

I think that things are more or less clear; however, I still do not understand why the references are needed. I have copy constructor defined so there is no problem that the overloaded operators obtain classes as arguments (The thing staying on the RHS of the operator is passed as an argument and the thing on the LHS calling the operator is passed as *this). Also the returned value should be fine because the copy constructor takes care of the temporary object that is being returned by the function end destroyed at the end. Or maybe this is just an extra secure code? I would normally use the 'const test& ' thing if i hadn't defined the copy constructor. I have seen that in books they do it as you suggested.

[Disch]

When you pass an object by value, it might create a copy (which in your case results in extra memory being allocated/freed). Since you don't really need an actual copy, passing by const reference is a way to optimize it so the copy can be avoided.

The compiler might be able to optimize out the copy so it might not make any difference, but passing by const reference can't really hurt.

As for making the + operator a const function... that's a const correctness issue that actually could impact code.

For example:

const test a(1,2);
const test b(3,4);

test c = a + b;  // will give you a compiler error unless the + operator is const