Overloaded assignment operator causes warning about recursion

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?

regarding the update: You should almost never have reference members. Ideally, only classes whose sole purpose is to manage a single unmanaged resource should have user defined destructors, copy constructors, and copy assignment operators. Can you try to describe your actual class design? From that we can try do deduce a better implementation. — Philipp, Dec 26 '11 at 11:16
This post has too many questions. SO uses a Q&A format. Distinct questions should be posted separately. The question about how to deal with reference members in the assignment operator and copy constructor should be a separate question (though search first to make sure it's not been asked before). — outis, Dec 26 '11 at 11:33

Tom · Accepted Answer · 2011-12-26T11:10:46.673

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

score 4 · Answer 2 · edited May 23 '17 at 12:11

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);
}

edited May 23 '17 at 12:11

Community

1
1

answered Dec 26 '11 at 11:09

Philipp

48,066
12
84
109

score 1 · Answer 3 · answered Dec 26 '11 at 10:44

1

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;
}

answered Dec 26 '11 at 10:44

Sergey Kalinichenko

714,442
84
1,110
1,523

score 0 · Answer 4 · answered Dec 26 '11 at 11:20

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.

Overloaded assignment operator causes warning about recursion

4 Answers4

Linked