Upon testing my C++11 code I encountered a bug whose origin is unclear to me. I have managed to reproduce the bug in the following contrived program.
#include
class Vector {
public:
double* data;
Vector(int n) {data = new double[n];};
~Vector() {delete[] data;}
};
Vector someMethod() {
if (true) {
Vector mat {3};
if (true) {
mat.data[0] = 0.1;
mat.data[1] = 0.2;
mat.data[2] = 0.3;
}
return mat;
}
}
int main() {
Vector mat { someMethod() };
std::cout << mat.data[0] << std::endl;
std::cout << mat.data[1] << std::endl;
std::cout << mat.data[2] << std::endl;
return 0;
}
The following output is produced by the program:
0
0.2
0.3
*** Error in `./testy': double free or corruption (fasttop): 0x0000000001f75010 ***
Aborted (core dumped)
Whereas the output should be:
0.1
0.2
0.3
It appears that the first value is corrupted. I experimented with different Vector lengths, it is always the case that only the first value is corrupted. I have failed to come up with a satisfactory explanation to the above behavior. I suspect it is due to the fact that the Vector object is declared and initialized in the block scope of an if() statement which is in the block scope of someMethod(). However I do not understand why that should be a problem, if indeed it is.
EDIT
Both solutions presented, the first following the rule of 3/5 and the second following the rule of 0, worked. Thank you! However I am still puzzled as to why this behavior occurs. What mechanism causes the the value to be corrupted? When I invoke the default move constructor in defining the Vector object at the start of the main method, what in its default implementation causes this behavior?
Solved
someMethod produces Vector mat. This Vector is copied to the Vector mat inside main. Since you didn't specify a copy constructor the compiler gives you the default copy constructor, which just copies every element that Vector has, which is double * data. After the copy both Vectors have a pointer to the same data. Then the original mat inside someMethod is destroyed, meaning its destructor runs, which deletes the data of both vectors. Once someMethod returns you get a Vector mat with an invalid data pointer. You then print out the invalid memory, causing undefined behavior. In this particular case the undefined behavior decided to give you slightly wrong output, but it could just as easily caues a segmentation fault.
I hope now it is clear why the fix from zenith corrects the issue.
You need to follow the rule of three/five/zero, which says
If a class requires a user-defined destructor, a user-defined copy constructor, or a user-defined copy assignment operator, it almost certainly requires all three.
In C++11, it's five instead of three because of the addition of the move constructor and move assignment operator.
So your class should look like this with the addition of appropriate copy constructor and assignment operator:
class Vector {
public:
double* data;
Vector(int n) {data = new double[n];}
// copy constructor
Vector(Vector const& source) {
data = new double[ /* the same `n` as used in to allocate *source.data */ ];
// copy *data from `source` to this->data
}
// copy assignment operator
Vector& operator=(Vector const& source) {
delete[] data;
data = new double[ /* the same `n` as used in to allocate *source.data */ ];
// copy *data from `source` to this->data
return *this;
}
~Vector() {delete[] data;}
};
You can add move operators to make it more efficient if you want.
To summarize the rule of zero:
Classes that have custom destructors, copy/move constructors or copy/move assignment operators should deal exclusively with ownership (which follows from the Single Responsibility Principle). Other classes should not have custom destructors, copy/move constructors or copy/move assignment operators.
It would look like this:
#include
class Vector {
public:
std::unique_ptr data;
Vector(int n) {data.reset(new double[n]);}
// no need to implement move constructor and move assignment operator,
// the automatically generated ones do the Right Thing because
// the resource is managed by the unique_ptr
// copy constructor and copy assignment operator are deleted
// because unique_ptr enforces the uniqueness of its managed
// resource
// if we wanted, we could implement the copy operators to do
// a deep copy of `data`, then we would just need to declare
// the move operators `= default` to not have them deleted.
// no need for destructor: unique_ptr manages the resource
};
No comments:
Post a Comment