Local variable deletes memory of another variable when going out of scope

Question

While designing a class that dynamically allocates memory I ran into the following problem regarding memory allocation. I was hoping that some of you might be able to point me in the right direction as to how I should design my class in a better way. My class dynamically allocates memory and therefore also deletes it in its destructor.

In order to illustrate the problem, consider the following silly class declaration:

class testClass{
    int* data;
public:
    testClass(){
        data = new int;
        *data = 5;
    }
    ~testClass(){
        delete data;
    }
};

So far so good. Now suppose that I create one of these objects in main

int main(){
    testClass myObject;
    return 0;
}

Still no issues of course. However, suppose that I now write a function that takes a testClass object as an input and call this from main.

void doNoting(testClass copyOfMyObject){
    //do nothing
}
int main(){
    testClass myObject;
    doNothing(myObject);
    return 0;
}

This time around, the function creates a local variable, copyOfMyObject, that's simply a copy of myObject. Then when the end of that function is reached, that local object automatically has its destructor called which deletes the memory pointed to by its data pointer. However, since this is the same memory pointed to by myObject's data pointer, myObject inadvertently has its memory deleted in the process. My question is: what is a better way to design my class?

http://stackoverflow.com/questions/4172722/what-is-the-rule-of-three — Benjamin Lindley, Mar 05 '13 at 18:38

Kristopher Johnson · Accepted Answer · 2013-03-05T19:12:35.523

7

When you call doNothing(), it is making a copy of your testClass object, because it is being passed by value. Unfortunately, when this copy is destroyed, it calls the destructor, which deletes the same data used by the original instance of testClass.

You want to learn about "copy constructors", and "passing by reference". That is, you should define a copy constructor for your class so that when a copy is made of an instance, it allocates its own memory for its data member. Also, rather than passing by value, you could pass a pointer or a reference to doNothing(), so that no copy is made.

edited Mar 05 '13 at 19:12

answered Mar 05 '13 at 18:39

Kristopher Johnson

81,409
55
245
302

That makes perfect sense! I can't believe that I didn't realize that's what was going on... I'll make sure to implement a copy constructor. Thanks for the quick feedback! – pontus Mar 05 '13 at 18:51

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

2

You should create a copy constructor, that is a constructor of the form:

testClass::testClass(const testClass &o)
{ 
    // appropriate initialization here
}

In your case, "appropriate initialization" might mean allocate a new chunk of memory and copy the memory from the old chunk into the new chunk. Or it may mean doing reference counting. Or whatever.

You should also read more about the Rule of Three right here on StackOverflow!

edited May 23 '17 at 11:58

Community

1
1

answered Mar 05 '13 at 18:40

Nik Bougalis

10,495
1
21
37

+1; reference counting was my initial idea – John Dvorak Mar 05 '13 at 18:54

score 0 · Answer 3 · edited Jun 20 '20 at 09:12

Here's a guideline from an authority: A class with any of {destructor, assignment operator, copy constructor} generally needs all 3

You need a copy constructor that will make a new allocated int for your data, that will then destruct that, but not affect the original.

Alternately, you can make a private copy constructor that's blank, which effectively disables it, forcing your users to pass by reference, or another non-copying way of doing things.

Local variable deletes memory of another variable when going out of scope

3 Answers3