Right usage of destructor

Question

I just started working with C++ and now I have a really basic question.

I wrote 2 classes:

Coordinate:

#include <stdio.h>

class Coordinate {
private:
    int x;
    int y;
public:
    Coordinate(int a, int b) {
        x = a;
        y = b;
    };
    void printTest() {
        printf("%d %d\n", x, y);
    };
};

Test:

class Test {
private:
    int q;
    Coordinate *point;
public:
    Test(int a, int b, int c) {
        q = a;
        point = new Coordinate(b, c);
    };
    virtual ~Test() {
        delete point;
    }
};

main function:

int main() {
    Test *test = new Test(1, 2, 3);
    // ...
    delete test;
    return 0;
}

In my main I worked with an object of the Test class. I wrote my own Test destructor but I am not sure if this destructor work like expected. Does it completly deallocte the memory from test? Or do I have to do something with the q attribute to deallocate it?

Note that you could avoid this by not using pointers where they aren't needed. `Test test(1, 2, 3);` and `Coordinate point;` work as is without any extra effort, though `point` would require properly using a member initializer list instead of attempting to construct a default and then reassign it. — chris, Mar 20 '17 at 17:51
why are you using pointers and dynamic memory allocation all over the place? The right usage of your destructor would be not to use it, ie it gets called automatically if you didnt use manual memory allocation. In short: use `Test test = Test(1,2,3);` instead of `Test* test= new ...` — 463035818_is_not_an_ai, Mar 20 '17 at 17:51
Off topic but in this example there is no reason to use pointers, `new`, and `delete`. C++ supports value semantics which makes object cleanup automatic. — NathanOliver, Mar 20 '17 at 17:51
The general rule, is one `delete` for every `new`. `q` is automatically allocated and will therefore be automatically deallocated. — George, Mar 20 '17 at 17:52
@George my personal rule is zero deletes for zero news (with very few exceptions) ;) — 463035818_is_not_an_ai, Mar 20 '17 at 17:52
It is also good to take a look at smart pointers, I.e. `std::shared_ptr` and `std::unique_ptr`. They are efficient and you then don't have to bother with `delete`. — JHBonarius, Mar 20 '17 at 17:59

Toby Speight · Answer 1 · 2017-03-20T18:19:05.943

What you've done is correct, as far as it goes. Valgrind reports

==28151== HEAP SUMMARY:
==28151==     in use at exit: 0 bytes in 0 blocks
==28151==   total heap usage: 3 allocs, 3 frees, 72,736 bytes allocated
==28151== 
==28151== All heap blocks were freed -- no leaks are possible

What you're missing is that the compiler has provided you with a default copy constructor and assignment operator. These will copy the pointer, rather than creating a new pointed-to value, so any time you copy a Test object you will then have two objects whose destructors will both try to delete the same storage. That's a double-free, and it can ruin your day.

To avoid that, C++ programmers use the Rule of Three or Rule of Five when writing classes, or - even better - the Rule of Zero, which says you shouldn't do any new or delete except in a class that exists only to own the storage.

score 5 · Accepted Answer · edited May 23 '17 at 12:25

5

Yes, this is the correct usage of a C++ destructor (your destructor in Test does not need the `virtual` keyword as there is no inheritance in your example).

As a rule of thumb every new should be followed by a delete, but when you're using class and instantiation it becomes more subtle than that. Following the Rule of Three or Five, when a class uses dynamic memory you should redefine the class destructor to deallocate accordingly, which you did, so great!

In your program execution, when delete test is invoked it will first deallocate the dynamic memory of point, before deallocating the dynamic memory you set in your main function with your test attribute. In this way you're not leaking memory (yay!) and your memory management has been done accordingly :)

edited May 23 '17 at 12:25

Community

1
1

answered Mar 20 '17 at 17:50

Santiago Varela

2,199
16
22

1

Please also mention the Rule of Three [Five]. – Rakete1111 Mar 20 '17 at 17:55
Wow, that's an aggressive title. I don't know if the virtual keyword needs a mention, i'd imagine the op probably know's what it means and I don't think it detracts from the "correctness" of the op's use of a destrcutor. – George Mar 20 '17 at 18:00
1

afaik "every new needs a delete" isnt part of the rule of 3/5, even if c++ didnt have a copy constructor or copy assignment, you would still need exactly one delete for each new – 463035818_is_not_an_ai Mar 20 '17 at 18:02
@George you may be right, but I'd like to think we're aiming for **semantically correct** code here and the virtual keyword is unnecessary. – Santiago Varela Mar 20 '17 at 18:02
3

it isnt unnecessary if the class is meant to be inherited from – 463035818_is_not_an_ai Mar 20 '17 at 18:02
@tobi303 +1 you're completely right! Noted and edited. – Santiago Varela Mar 20 '17 at 18:05
@tobi303 there is no inheritance here...this is a concrete case and question. – Santiago Varela Mar 20 '17 at 18:06
not that i care so much, but imho the `virtual` is just as unnecessary in that code as the `private:` and more a matter of taste than of correctness – 463035818_is_not_an_ai Mar 20 '17 at 18:09

Rama · Answer 3 · 2017-03-20T18:23:07.517

You need a copy constructor to be sure that memory management is doing ok. Because implicitly-generated constructors and assignment operators simply copy all class data members ("shallow copy"). And since you have pointers in your class with allocated data you really need it.

For example if in your part of main code: // ... you do a copy like:

Test testB = *test;

testB has a Coordinate pointer that is pointing to the same memory area that *test. It could cause problems, for example, when testB goes out of scope, it will free the same memory that *test is using.

Copy constructor should look like:

Test(const Test& other)
    : point (new Coordinate(other.x, other.y))
    , q(other.q)
{

}

With this you will be sure that every Coordinate* is initializated ok and released ok.

score 0 · Answer 4 · answered Mar 20 '17 at 23:14

In your case, you don't need pointer

class Test {
private:
    int q;
    Coordinate point;
public:
    Test(int a, int b, int c) : q(a), point(b, c) {};
};

int main() {
    Test test(1, 2, 3);
    // ...
    return 0;
}

Would be enough.

If you want to allocate memory, I strongly suggest to use smart pointer or container instead:

class Test {
private:
    int q;
    std::unique_ptr<Coordinate> point;
public:
    Test(int a, int b, int c) : q(a), point(std::make_unique_ptr<Coordinate>(b, c)) {};
};

int main() {
    auto test = std::make_unique<Test>(1, 2, 3);
    // ...
    return 0;
}

In both way, you respect rule of 3/5/0.

In second case, you should probably want to provide copy constructor to your class:

Test(const Test& rhs) : q(rhs.q), point(std::make_unique<Coordinate>(*rhs.point)) {}

Right usage of destructor

4 Answers4

Yes, this is the correct usage of a C++ destructor (your destructor in Test does not need the virtual keyword as there is no inheritance in your example).

Yes, this is the correct usage of a C++ destructor (your destructor in Test does not need the `virtual` keyword as there is no inheritance in your example).