Overloaded assignment operator causes warning abou

Question

I need to implement the overloaded the assignment operator in a class so the vector.erase function will work properly as proposed in the answers to "vector::erase with pointer member". I have implemented also a copy constructor for the same purpose. By the following implementation of the operator I get the warning :

'Player::operator=' : recursive on all control paths, function will cause runtime stack overflow.

Apparently the implementation of Player::operator= is incorrect. What is the correct implementation?

//Copy constructor:
Player::Player(const Player& otherPlayer) {
   ...
}


Player& Player::operator=(const Player& rhs) {
    *this = Player(rhs);
    return *this;
}

Does the erase function of a multimap work on the same way as the vector? When I use in the multimap I do not receive errors about not implementing the overloaded operator= as it happens with vector. What is the truth?

Also, the Player has a reference to a Bank as a member. Should I do the assignment of the reference just by =? What is then the purpose of the copy constructor?

Answer 1

In answer to

What is then the purpose of the copy constructor?

The purpose is to give you the fullest control over what the copying does.
For instance, if your class contains a char* member, and you make a copy, you might want to keep the char* the same value as the one in the instance you're copying (i.e. a pointer that points to the same location of the original), or you might want to make a duplicate of the string it points to.
The compiler doesn't know by itself which of these choices you want, so that's where you come in.

Answer 2

The problem is that you are calling = from inside the =. Instead of this, you should assign elements of your class field by field, like this:

Player& Player::operator=(const Player& rhs) {
    name = rhs.name;
    score = rhs.score;
    return *this;
}

Answer 3

Your problem is this line:

*this = Player(rhs);

This calls the overloaded = method again, resulting in a recursive loop. You should assign all members separately:

x = rhs.x;
y = rhs.y; // for example

EDIT Also note that as the object you are passing is the same type of object the instance method belongs to, you can access private members just like you normally would for public members.

EDIT2 As far as I am aware you would not want to copy a reference, because that means the same Bank object would be accessible from both objects which breaks encapsulation and would probably cause some unintended problems. Is there any specific reason you are not using pointers? If the bank object was a pointer, you could overload the = operator for the bank, and then assign it normally:

bank = rhs.bank;

As for the difference of Copy Constructors and Assignment Operators, a quick google search turned up this link: http://prashanth-cpp.blogspot.com/2007/01/assignment-operator-vs-copy-constructor.html

Answer 4

A good way to implement operator= correctly is the copy-and-swap idiom:

Player& Player::operator=(Player rhs) {  // note: pass by value
  this->swap(rhs);
  return *this;
}

void Player::swap(Player& other) {  // member function
  using std::swap;
  swap(this->name, other.name);
  swap(this->score, other.score);
}

void swap(Player& a, Player& b) {  // free function
  a.swap(b);
}