🔒 Locked

operator+ overloading problem

Started by danbis_uk Jul 21, 2004 at 6:54 AM 18 replies 3.9k views

Original Post

Jul 21, 2004 at 6:54 AM

I am having problems trying to figure out how to get my operator+ to work. I believe the problem is down to the fact that p is a pointer, but i cant for the life of me figure out how to solve it. I would be very grateful if someone would point me in the right direction. (as soon as i add (temp = a) to my + operator, my Point array messes up.)


class Obstacle{
    
	Point *p;
	int	n;

public:
	Obstacle();
	Obstacle(int);
	~Obstacle();
	Obstacle &operator+= (const Obstacle&);
	Obstacle &operator= (const Obstacle&);
	Obstacle &operator+ (Obstacle);		

};

Obstacle &Obstacle::operator+= (const Obstacle &a)
{
	Point *temp = new Point[n];
	for (int i = 0; i < n; i++) temp = p;
	if (p != NULL) delete[] p;
	p = new Point[n + a.n];
	for(int i = 0; i < n; i++) p = temp; 
	for(int i = 0; i < a.n; i++)p[n+i] = a.p;
	n+=a.n;
	if (temp != NULL) delete[] temp;
	return *this;
}

// overloaded = operator
Obstacle &Obstacle::operator= (const Obstacle &a)
{
	if(this != &a){
		if (p != NULL) delete[]p;
		p = new Point[a.n];
		for (int i = 0; i < a.n; i++)p = a.p;
		n = a.n;
	}
	return *this;
}


Obstacle &Obstacle::operator+ (Obstacle a)
 {
	
	Obstacle temp = Obstacle();

	temp = a;

	return temp;
}

Thanks, Dan

Kippesoep

Jul 21, 2004 at 7:08 AM

This is probably caused by invalid values for p. You don't show the default constuctor, but I would guess that it doesn't initialise p (and n) to 0.

Add something like

Obstacle () : p (0), n (0) {}

to your class declaration and it should work.

Also, in your operator + you're returning a reference to a temporary object, which is A Bad Thing. Make it look something like this:

Obstacle Obstacle::operator+ (const Obstacle &a){  Obstacle temp = *this;  temp += a;  return temp;}

Finally, note that checking for NULL is not necessary when deleting. Save yourself a lot of typing and just use "delete [] p".

Jingo

Jul 21, 2004 at 7:28 AM

There isn't usually a reason to make operator+ a member function

Obstacle operator+(Obstacle copy_ob_1, const Obstacle& ob_2){   copy_ob_1 += ob_2;   return copy_ob_1;}

Doing it like this gives you a better abstraction, and prevents the pollution of the inclass public interface, and also ensures that operator+= and operator+ behave consistently.

snk_kid

Jul 21, 2004 at 7:35 AM

Quote:
Original post by Jingo
There isn't usually a reason to make operator+ a member function

*** Source Snippet Removed ***

Doing it like this gives you a better abstraction, and prevents the pollution of the inclass public interface, and also ensures that operator+= and operator+ behave consistently.

Thats a little strange why are you passing one parameter by value and the other by reference? by doing that you end making extra copies, this is preferable:

class A {public:   A& operator+=(const A&) {      //....      return *this;   }};inline A operator+(const A& a, const A& b) {   return (A(a) += b);}

Jingo

Jul 21, 2004 at 7:58 AM

operator+ has to create a copy somewhere, otherwise it would be no different to operator+=, creating a copy in the parameter seems like as good a place as any to create it.

snk_kid

Jul 21, 2004 at 8:10 AM

Quote:
Original post by Jingo
operator+ has to create a copy somewhere, otherwise it would be no different to operator+=, creating a copy in the parameter seems like as good a place as any to create it.

But if any of the data members are pointers (as in the above example) and you've defined a proper copy constructor & assignemt operators you'll end up making 2 deep-copies with that method, once for the paramter pass, and then for using "+=" in your example.

Jingo

Jul 21, 2004 at 8:13 AM

Your method also creates 2 copies though, just in a different place. Both create 2 copies in 2003.net, both methods create 3 copies in vc++6.

snk_kid

Jul 21, 2004 at 8:15 AM

Quote:
Original post by Jingo
Your method also creates 2 copies though, just in a different place. Both create 2 copies in 2003.net, both methods create 3 copies in vc++6.

Sorry i was meant to say 3 copies not 2, third being on return if it isn't optimized, where as the one i posted will 2 and may even be one if optimized.

Jingo

Jul 21, 2004 at 8:19 AM

Well they could both be implemented with one copy if the return value optimization is used. The only difference between the 2 methods is that I used the named variable created in the passing of the parameter, and you use the unnamed variable created in the += expression, both can be opitmized away.

Direct4D

Jul 21, 2004 at 8:25 AM

Quote:
Original post by snk_kid
Quote:
Original post by Jingo
There isn't usually a reason to make operator+ a member function

*** Source Snippet Removed ***

Doing it like this gives you a better abstraction, and prevents the pollution of the inclass public interface, and also ensures that operator+= and operator+ behave consistently.

Thats a little strange why are you passing one parameter by value and the other by reference? by doing that you end making extra copies, this is preferable:
class A {public:   A& operator+=(const A&) {      //....      return *this;   }};inline A operator+(const A& a, const A& b) {   return (A(a) += b);}

Only i would make that operator+ return by const value, like this:

inline const A operator+(A a, const A& b) {   return (a += b);}

Reason:

Quote:
quoted from: Bruce Eckel - Thinking in CPP Volume 1
Return by value as const

Returning by value as a const can seem a bit subtle at first, so it deserves a bit more explanation. Consider the binary operator+. If you use it in an expression such as f(a+b), the result of a+b becomes a temporary object that is used in the call to f( ). Because it’s a temporary, it’s automatically const, so whether you explicitly make the return value const or not has no effect.

However, it’s also possible for you to send a message to the return value of a+b, rather than just passing it to a function. For example, you can say (a+b).g( ), in which g( ) is some member function of Integer, in this case. By making the return value const, you state that only a const member function can be called for that return value. This is const-correct, because it prevents you from storing potentially valuable information in an object that will most likely be lost.

danbis_uk

Jul 21, 2004 at 8:48 AM

thanks for all the help. I sort of managed to get it working. It returns the right values, but i get a debug assertion failure when my program finishes. I must have a memory leak somewhere.

adding inline onto the operator+ function enabled me to recieve the right point values but also seems to cause the error.

You can now see the reason that i check for NULL before deleting. It is probably a little pointless setting the pointer to be NULL in the constructor though

class Obstacle{    	Point *p;	int	n;public:	Obstacle();	~Obstacle();	const Obstacle &operator+= (const Obstacle&);	const Obstacle &operator= (const Obstacle&);	void addPoint(int, int);};// Implementation ////////////////////////////////////////// ConstructorObstacle::Obstacle(){	p = NULL;	n = 0;}// DestructorObstacle::~Obstacle(){	if (p != NULL) delete[] p;}// overloaded += operatorconst Obstacle &Obstacle::operator+= (const Obstacle &a){	Point *temp = new Point[n];	for (int i = 0; i < n; i++) temp = p;	if (p != NULL) delete[] p;	p = new Point[n + a.n];	for(int i = 0; i < n; i++) p = temp; 	for(int i = 0; i < a.n; i++)p[n+i] = a.p;	n+=a.n;	delete[] temp;	return *this;}// overloaded = operatorconst Obstacle &Obstacle::operator= (const Obstacle &a){	if(this != &a){		if (p != NULL) delete[]p;		p = new Point[a.n];		for (int i = 0; i < a.n; i++)p = a.p;		n = a.n;	}	return *this;}inline const Obstacle operator+(Obstacle &a, const Obstacle &b){   	return a+=b;}// Adds the Point(x,y) onto the end of the array of points pvoid Obstacle::addPoint(int x, int y){	Point *temp = new Point[n];	for (int i = 0; i < n; i++) temp = p;	if (p != NULL) delete[] p;	p = new Point[n+1];	for (int i = 0; i < n; i ++) p = temp;	p[n].x = x;	p[n].y = y;	if (temp != NULL)delete[] temp;	n++;}

thanks,
Dan

Jingo

Jul 21, 2004 at 8:53 AM

What are you doing in the objects constructors/destructors?

Post the code.

danbis_uk

Jul 21, 2004 at 8:56 AM

I just did. Scroll down and they are the first two functions.

Kippesoep

Jul 21, 2004 at 9:04 AM

Quote:
Original post by danbis_uk
You can now see the reason that i check for NULL before deleting.

There is no reason to check for NULL before deleting. Deleting a NULL pointer is a valid operation (which does absolutely nothing and will not crash). If you get a problem with your delete operation, then it is because you're either deleting something that shouldn't be (invalid pointer or already deleted) or memory has been overwritten somewhere.

danbis_uk

Jul 21, 2004 at 9:12 AM

Thanks for that Kippesoep. I have removed all checks for Null pointers.
Unfortunatley there is something else causing memory problems. It only crashes on completion of the program.

Jingo

Jul 21, 2004 at 9:13 AM

Well you implemented operator+ incorrectly, the first argument should not have a reference parameter if you use the method i suggested, probably better using snk_kids version, it is clearer.

The code is pretty hard to follow, it looks like you are trying to implement a sizeable array of some kind, std::vector already exists for this purpose, would be better off using that, and you could forget about the memory worries you are having.

That said, you are doing a lot of needless copying in the code,

Point *temp = new Point[n];for (int i = 0; i < n; i++) temp = p;if (p != NULL) delete[] p;

for example, could be rewritten as

Point *temp = p;

In both functions. As mentioned above, the deleting of a null pointer is also fine, do not need to check for that in the code. The best advice i could give would be to use std::vector, or use the debugger to step through the code until you find the point at which it crashes.

danbis_uk

Jul 21, 2004 at 9:21 AM

ive managed to solve it by removing 'delete[] p' from the destructor and therefore leaving it empty. I have no idea why though. Wouldn't this cause problems later on?

Jingo

Jul 21, 2004 at 9:25 AM

Yes it would be a problem, it would leak all the memory.

The problem is no doubt inside your copy constructor, you need to define one to perform a deep copy, the implicit one is just copying the pointer to the address, and your destructor is calling operator delete on the memory twice, causing the program to crash.

Jingo

Jul 21, 2004 at 9:28 AM

If you insist on not using std::vector, this code should do it.

Obstacle::Obstacle(const Obstacle &a):p(new Point[a.n]), n(a.n){  for (int i = 0; i < n; ++i)     p = a.p;  return *this;}

danbis_uk

Jul 21, 2004 at 9:29 AM

aha that makes sense, i shall work on a copy constructor.
Thanks very much

well i would have done if you hadn't already done it for me:)
thanks

Topic Locked

This topic has been locked by a moderator. New replies are not allowed.