Building on some of the already posed answers: As mentioned before, you need a destructor to make sure pi is released. Moreover, if you provide no Copy constructor or assignment operator, the compiler will create these for you. Both will simply copy pi over without checking, und thus also produce a memory leak. This means you need to define both a copy constructor and an assignment operator yourself, to prevent memory leaks from inadvertent copying! Another, minor, issue is that you didn't think to initialize the integer pi is pointing to. I will ignore the apparent uselessness of the class in it's current state, assuming that it's going to be expanded later. Going with the above, I would rewrite this to sth like that:
class A {
int* pi;
public:
int* getpi() {
if (0 == pi)
pi = new int();
if (pi != 0)
*pi = 0;
return pi;
}
A() : pi(0) {}
A(const A& other) : pi(0) {
*getpi() = *other.pi; /* deep copy */
}
~A() { delete pi; }
A& operator=(const A& other) {
*getpi() = *other.pi; /* deep copy */
return *this;
}
}
Note that you could also implement a shallow copy by just redirecting pi to wherever the other object's pi is pointing, but if you do that, you need some mechanism such as reference counting to prevent deleting an int object while oter instances are still pointing to it! Note also that I only programmed the happy case with respect to allocation: if getpi() ever fails to allocate pi (and thus returns 0), then the call to the copy constructor or assignment operator will throw a NULL pointer exception. You might want to catch that, especially in the constructor. [edit]Made getpi() public to resolve the inaccessibility issue, and added initializion of *pi[/edit]
modified on Monday, January 31, 2011 6:02 AM
