Does this C++ code leak memory?
assignment-operator, c++, memory-leaks
Solution
Whether this leaks memory? No it doesn't.
It seems most of the people have missed the point here. So here is a bit of clarification.
The initial response of "No it doesn't leak" in this answer was Incorrect but the solution that was and is suggested here is the only and the most appropriate solution to the problem.
The solution to your woes is:
Not use a pointer to integer member(`int *`) but to use just an integer (`int`), You don't really need dynamically allocated pointer member here. You can achieve the same functionality using an `int` as member. Note that in C++ You should use `new` as little as possible.
If for some reason(which I can't see in the code sample) You can't do without dynamically allocated pointer member read on:
You need to follow the Rule of Three!
Why do you need to follow Rule of Three?
The Rule of Three states:
If your class needs either
a copy constructor, an assignment operator, or a destructor,
then it is likely to need all three of them.
Your class needs an explicit destructor of its own so it also needs an explicit copy constructor and copy assignment operator. Since copy constructor and copy assignment operator for your class are implicit, they are implicitly public as well, Which means the class design allows to copy or assign objects of this class. The implicitly generated versions of these functions will only make a shallow copy of the dynamically allocated pointer member, this exposes your class to:
- Memory Leaks &
- Dangling pointers &
- Potential Undefined Behavior of double deallocation
Which basically means you cannot make do with the implicitly generated versions, You need to provide your own overloaded versions and this is what Rule of Three says to begin with.
The explicitly provided overloads should make a deep copy of the allocated member and it thus prevents all your problems.
How to implement the Copy assignment operator correctly?
In this case the most efficient and optimized way of providing a copy assignment operator is by using: copy-and-swap Idiom @GManNickG's famous answer provides enough detail to explain the advantages it provides.
Suggestion:
Also, You are much better off using smart pointer as an class member rather than a raw pointer which burdens you with explicit memory management. A smart pointer will implicitly manage the memory for you. What kind of smart pointer to use depends on lifetime and ownership semantics intended for your member and you need to choose an appropriate smart pointer as per your requirement.
Problem
``` struct Foo { Foo(int i) { ptr = new int(i); } ~Foo() { delete ptr; } int* ptr; }; int main() { { Foo a(8); Foo b(7); a = b; } //Do other stuff } ``` If I understand correctly, the compiler will automatically create an assignment operator member function for `Foo`. However, that just takes the value of `ptr` in `b` and puts it in `a`. The memory allocated by `a` originally seems lost. I could do a call `a.~Foo();` before making the assignment, but I heard somewhere that you should rarely need to explicitly call a destructor. So let's say instead I write an assignment operator for `Foo` that deletes the `int` pointer of the left operand before assigning the r-value to the l-value. Like so: ``` Foo& operator=(const Foo& other) { //To handle self-assignment: if (this != &other) { delete this->ptr; this->ptr = other.ptr; } return *this; } ``` But if I do that, then when `Foo a` and `Foo b` go out of scope, don't both their destructors run, deleting the same pointer twice (since they both point to the same thing now)? Edit: If I understand Anders K correctly, this is the proper way to do it: ``` Foo& operator=(const Foo& other) { //To handle self-assignment: if (this != &other) { delete this->ptr; //Clones the int this->ptr = new int(*other.ptr); } return *this; } ``` Now, `a` cloned the `int` that `b` pointed to, and sets its own pointer to it. Perhaps in this situation, the `delete` and `new` were not necessary because it just involves `int`s, but if the data member was not an `int*` but rather a `Bar*` or whatnot, a reallocation could be necessary. Edit 2: The best solution appears to be the copy-and-swap idiom.