Home » Community » Forums » General Programming » operator+ overloading problem
  Intel sponsors gamedev.net search:   
[Control Panel] [Register] [Bookmarks] [Who's Online] [Active Topics] [Stats] [FAQ] [Search]

Add Forum to Favorites |  Send Topic To a Friend | View Forum FAQ | Track this topic
 Last Thread Next Thread 
 operator+ overloading problem
Post New Topic  Post Reply 
Reply Quoting This MessageEdit Message danbis_uk    Member since: 7/21/2004  From: United Kingdom
 Posted - 7/21/2004 7:54:49 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[i] = p[i];
	if (p != NULL) delete[] p;
	p = new Point[n + a.n];
	for(int i = 0; i < n; i++) p[i] = temp[i]; 
	for(int i = 0; i < a.n; i++)p[n+i] = a.p[i];
	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[i] = a.p[i];
		n = a.n;
	}
	return *this;
}


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

	temp = a;

	return temp;
}


Thanks,
Dan
 User Rating: 1015   |  Rate This User  Send Private MessageView Profile Report this Post to a Moderator | Link

Reply Quoting This MessageEdit Message Kippesoep    Member since: 10/25/2001  From: Etten, Netherlands
 Posted - 7/21/2004 8:08:24 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".

Kippesoep
 User Rating: 1528   |  Rate This User  Send Private MessageView Profile Report this Post to a Moderator | Link

Reply Quoting This MessageEdit Message Jingo    Member since: 11/2/2003  From: Bath, United Kingdom
 Posted - 7/21/2004 8:28:42 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.
 User Rating: 1318   |  Rate This User  Send Private MessageView Profile Report this Post to a Moderator | Link

Reply Quoting This MessageEdit Message snk_kid    Member since: 5/22/2003  From: United Kingdom
 Posted - 7/21/2004 8:35:56 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);
}


 User Rating: 1803   |  Rate This User  Send Private MessageView Profile Report this Post to a Moderator | Link

Reply Quoting This MessageEdit Message Jingo    Member since: 11/2/2003  From: Bath, United Kingdom
 Posted - 7/21/2004 8:58:04 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.
 User Rating: 1318   |  Rate This User  Send Private MessageView Profile Report this Post to a Moderator | Link

Reply Quoting This MessageEdit Message snk_kid    Member since: 5/22/2003  From: United Kingdom
 Posted - 7/21/2004 9:10:42 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.
 User Rating: 1803   |  Rate This User  Send Private MessageView Profile Report this Post to a Moderator | Link

Reply Quoting This MessageEdit Message Jingo    Member since: 11/2/2003  From: Bath, United Kingdom
 Posted - 7/21/2004 9:13:09 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.
 User Rating: 1318   |  Rate This User  Send Private MessageView Profile Report this Post to a Moderator | Link

Reply Quoting This MessageEdit Message snk_kid    Member since: 5/22/2003  From: United Kingdom
 Posted - 7/21/2004 9:15:58 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.
 User Rating: 1803   |  Rate This User  Send Private MessageView Profile Report this Post to a Moderator | Link

Reply Quoting This MessageEdit Message Jingo    Member since: 11/2/2003  From: Bath, United Kingdom
 Posted - 7/21/2004 9:19:33 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.
 User Rating: 1318   |  Rate This User  Send Private MessageView Profile Report this Post to a Moderator | Link

Reply Quoting This MessageEdit Message Direct4D    Member since: 11/27/2003  From: The Netherlands
 Posted - 7/21/2004 9:25:11 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.
 User Rating: 1038   |  Rate This User  Send Private MessageView Profile Report this Post to a Moderator | Link

Reply Quoting This MessageEdit Message danbis_uk    Member since: 7/21/2004  From: United Kingdom
 Posted - 7/21/2004 9:48:57 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 ////////////////////////////////////////

// Constructor
Obstacle::Obstacle()
{
	p = NULL;
	n = 0;
}


// Destructor
Obstacle::~Obstacle()
{
	if (p != NULL) delete[] p;
}

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

// overloaded = operator
const 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[i] = a.p[i];
		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 p
void Obstacle::addPoint(int x, int y)
{
	Point *temp = new Point[n];
	for (int i = 0; i < n; i++) temp[i] = p[i];
	if (p != NULL) delete[] p;
	p = new Point[n+1];
	for (int i = 0; i < n; i ++) p[i] = temp[i];
	p[n].x = x;
	p[n].y = y;
	if (temp != NULL)delete[] temp;
	n++;
}


thanks,
Dan
 User Rating: 1015   |  Rate This User  Send Private MessageView Profile Report this Post to a Moderator | Link

Reply Quoting This MessageEdit Message Jingo    Member since: 11/2/2003  From: Bath, United Kingdom
 Posted - 7/21/2004 9:53:39 AM
What are you doing in the objects constructors/destructors?

Post the code.
 User Rating: 1318   |  Rate This User  Send Private MessageView Profile Report this Post to a Moderator | Link

Reply Quoting This MessageEdit Message danbis_uk    Member since: 7/21/2004  From: United Kingdom
 Posted - 7/21/2004 9:56:28 AM
I just did. Scroll down and they are the first two functions.
 User Rating: 1015   |  Rate This User  Send Private MessageView Profile Report this Post to a Moderator | Link

Reply Quoting This MessageEdit Message Kippesoep    Member since: 10/25/2001  From: Etten, Netherlands
 Posted - 7/21/2004 10:04:13 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.


Kippesoep
 User Rating: 1528   |  Rate This User  Send Private MessageView Profile Report this Post to a Moderator | Link

Reply Quoting This MessageEdit Message danbis_uk    Member since: 7/21/2004  From: United Kingdom
 Posted - 7/21/2004 10:12:19 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.
 User Rating: 1015   |  Rate This User  Send Private MessageView Profile Report this Post to a Moderator | Link

Reply Quoting This MessageEdit Message Jingo    Member since: 11/2/2003  From: Bath, United Kingdom
 Posted - 7/21/2004 10:13:45 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[i] = p[i];
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.
 User Rating: 1318   |  Rate This User  Send Private MessageView Profile Report this Post to a Moderator | Link

Reply Quoting This MessageEdit Message danbis_uk    Member since: 7/21/2004  From: United Kingdom
 Posted - 7/21/2004 10:21:37 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?
 User Rating: 1015   |  Rate This User  Send Private MessageView Profile Report this Post to a Moderator | Link

Reply Quoting This MessageEdit Message Jingo    Member since: 11/2/2003  From: Bath, United Kingdom
 Posted - 7/21/2004 10:25:08 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.
 User Rating: 1318   |  Rate This User  Send Private MessageView Profile Report this Post to a Moderator | Link

Reply Quoting This MessageEdit Message Jingo    Member since: 11/2/2003  From: Bath, United Kingdom
 Posted - 7/21/2004 10:28:14 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[i] = a.p[i];
  return *this;
}


 User Rating: 1318   |  Rate This User  Send Private MessageView Profile Report this Post to a Moderator | Link

Reply Quoting This MessageEdit Message danbis_uk    Member since: 7/21/2004  From: United Kingdom
 Posted - 7/21/2004 10:29:09 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
 User Rating: 1015   |  Rate This User  Send Private MessageView Profile Report this Post to a Moderator | Link

All times are ET (US)

Post Reply		  Last Thread Next Thread 
Forum Rules:
You may not post new threads
You may post replies
You may not edit your posts
You may not use HTML in your posts
Jump To:
Administrative Options: