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?

Streak

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. – Chlortetracycline

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 ... – Cavesson

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. – Endlong

The general rule, is one delete for every new. q is automatically allocated and will therefore be automatically deallocated. – Lydgate

@Lydgate my personal rule is zero deletes for zero news (with very few exceptions) ;) – Cavesson

@MohammadTayyab That is not right at all – Endlong

@MohammadTayyab there is nothing virtual here – Cavesson

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. – Insufflate

Answer 1

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 :)

Alec

Please also mention the Rule of Three [Five]. – Pustule

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. – Lydgate

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 – Cavesson

@Lydgate you may be right, but I'd like to think we're aiming for semantically correct code here and the virtual keyword is unnecessary. – Alec

it isnt unnecessary if the class is meant to be inherited from – Cavesson

@tobi303 +1 you're completely right! Noted and edited. – Alec

@tobi303 there is no inheritance here...this is a concrete case and question. – Alec

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 – Cavesson

Answer 2

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.

Tiertza

Answer 3

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.

Genna

Answer 4

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)) {}

Esquire