Kilpo
Kilpo

Reputation: 61

What is the best way for deleting dynamic allocated memory of class member

The following code is just a raw example reference. Everything works fine, but when the program is about to close it stuck for about a few seconds. It's all because of the wrong way of deleting memory I am assuming. Please find below code snippet:

class Test {
private:
    static int count;
    static Test* arr;
public:
    int num;

    Test() {}

    Test(int val) : num(val) {
        arr[count] = *this;
        count++;
    }

    ~Test() {
        delete[] arr;
    }

    void print() {
        for (int a = 0; a <= count - 1; a++) {
            std::cout << arr[a].num << std::endl;
        }
    }
};

int Test::count = 0;
Test* Test::arr = new Test[2];

int main(int argc, char** argv) {
    Test t1(10);
    Test t2(20);
    t2.print();
    return 0;
}

Upvotes: 5

Views: 95

Answers (2)

walnut
walnut

Reputation: 22152

Don't use raw new and delete at all:

class Test {
private:
    static int count;
    static std::vector<Test> arr;
public:
    int num;

    Test() {}

    Test(int val) : num(val) {
        arr[count] = *this;
        count++;
    }

    // No user-defined destructor needed!

    void print() {
        for (int a = 0; a <= count - 1; a++) {
            std::cout << arr[a].num << std::endl;
        }
    }
};

int Test::count = 0;
std::vector<Test> Test::arr{2};

int main(int argc, char** argv) {
    Test t1(10);
    Test t2(20);
    t2.print();
    return 0;
}

(requires #include<vector>)

This does the same thing, but does not cause you any trouble with finding the correct place to delete. std::vector manages that for you.

Note however that whatever the purpose of this class is, it probably should not be limited in the number of Test it can create:

class Test {
private:
    static std::vector<Test> arr;
public:
    int num;

    Test() {}

    Test(int val) : num(val) {
        arr.push_back(*this); // Add new element!
    }

    // No user-defined destructor needed!

    void print() {
        for (int a = 0; a < arr.size(); a++) {
            std::cout << arr[a].num << std::endl;
        }
    }
};

int Test::count = 0;
std::vector<Test> Test::arr; // Start empty!

int main(int argc, char** argv) {
    Test t1(10);
    Test t2(20);
    t2.print();
    return 0;
}

Now you aren't limited to using only two elements and you don't have to keep track of count either!


If you want to use new (which you shouldn't), then you need to call delete[] exactly once for each new[].

In your code you call new[] once to initialize arr at program startup. You call delete[] each time a Test is destroyed and you have four Test instances in your program: 2 variables of type Test in main and 2 objects of type Test in the array that you allocate for arr. That means you are deleting the array pointed to by arr four times! That causes undefined behavior.

To do it correctly, you need to deallocate it once, when it is not needed anymore:

class Test {
private:
    static int count;
    static Test* arr;
public:
    int num;

    Test() {}

    Test(int val) : num(val) {
        arr[count] = *this;
        count++;
    }

    // No user-defined destructor needed!

    void print() {
        for (int a = 0; a <= count - 1; a++) {
            std::cout << arr[a].num << std::endl;
        }
    }
};

int Test::count = 0;
Test* Test::arr = new Test[2]; // Executed once at startup

int main(int argc, char** argv) {
    Test t1(10);
    Test t2(20);
    t2.print();

    delete[] arr; // We don't need the array anymore, so `delete[]` it *once*

    return 0;
}

Please also note that I doubt the you are approaching whatever problem you actually have the correct way with the design of this class. If your intention is to register all objects of type Test that are created in a global registry, then you should be warned that you are saving copies of the Test objects in arr.

Discussing other approaches to such a problem is out-of-scope of the question, especially if I don't know the actual intended use. So I will stop here.

Upvotes: 6

edtheprogrammerguy
edtheprogrammerguy

Reputation: 6039

You are deleting the static memory in arr multiple times which leads to undefined behavior.

Upvotes: 0

Related Questions