Is Calling Destructor Manually Always a Sign of Bad Design

Is calling destructor manually always a sign of bad design?

Calling the destructor manually is required if the object was constructed using an overloaded form of operator new(), except when using the "std::nothrow" overloads:

T* t0 = new(std::nothrow) T();
delete t0; // OK: std::nothrow overload

void* buffer = malloc(sizeof(T));
T* t1 = new(buffer) T();
t1->~T(); // required: delete t1 would be wrong
free(buffer);

Outside managing memory on a rather low level as above calling destructors explicitly, however, is a sign of bad design. Probably, it is actually not just bad design but outright wrong (yes, using an explicit destructor followed by a copy constructor call in the assignment operator is a bad design and likely to be wrong).

With C++ 2011 there is another reason to use explicit destructor calls: When using generalized unions, it is necessary to explicitly destroy the current object and create a new object using placement new when changing the type of the represented object. Also, when the union is destroyed, it is necessary to explicitly call the destructor of the current object if it requires destruction.

Is it a bad design decision to manually call the destructor in the following case?

Well, by e2 "eating" e1 it is actually taking ownership of it. Specifically, it has to take ownership of it, in order to destroy it. So the way to do it is actually this:

void Entity::eat(std::unique_ptr<Edible> subject) {
    this->energy += subject->getNutritionalInformation();
}

And... that's it. the subject will automatically be destroyed at the end of scope. In order to use this, you'll need to call std::move explicitly; this indicates that you are transferring ownership from the calling scope, into e2.

std::unique_ptr<Entity> e1(new Entity);
std::unique_ptr<Entity> e2(new Entity);

std::cout << "Pre feasting energy: " << e1->getNutritionalInformation() << std::endl;
e1->eat(std::move(e2));
std::cout << "After feasting energy: " << e1->getNutritionalInformation( << std::endl;

Note that after std::move has been called on e2, e2 can no longer be assumed to actually point to an entity (since ownership has been transferred).

Good or bad: Calling destructor in constructor

This code is ugly but legal. When an exception is thrown from constructor, the corresponding destructor is never called. So calling it manually before throwing is needed to prevent the resource leak. The real bug here is not calling destructor manually in other cases before throwing exception.

Of course, a better way of doing this is having a separate RAII object that encapsulates commHandle. A unique_ptr with custom deleter can serve this role.

Any destructor beyond low-level libraries is a code smell in modern C++.

Design: How to avoid calling destructor?

You should post some code so we can see exactly what is happening, but you're right that you shouldn't manually call the destructor because that will cause undefined behavior (see AliciaBytes' answer). Instead, add a method to the class to close the window, and use the class to provide a safe interface to the SDL window functions. Here is a sketch:

class Window {
private:
    SDL_Window *window_;

public:
    Window() : window_(nullptr) { }
    Window(const Window &) = delete;
    Window(Window &&w) : window_(w.window_) { w.window_ = nullptr; }
    Window(const char* title, int x, int y, int w, int h, Uint32 flags)
        : window(SDL_CreateWindow(title, x, y, w, h, flags))
    { }

    ~Window()
    {
        if (window_ != nullptr)
            SDL_DestroyWindow(window_);
    }

    Window &operator=(const Window &) = delete;
    Window &operator=(Window &&w)
    {
        if (window_) { SDL_DestroyWindow(window_); window_ = nullptr; }
        window_ = w.window_;
        w.window_ = nullptr;
    }

    operator bool() const
    {
        return window_ != nullptr;
    }

    void close()
    {
        if (window_ != nullptr) {
            SDL_DestroyWindow(window_);
            window_ = nullptr;
        }
    }
};

Is calling destructor manually always a sign of bad design?

Calling the destructor manually is required if the object was constructed using an overloaded form of operator new(), except when using the "std::nothrow" overloads:

T* t0 = new(std::nothrow) T();
delete t0; // OK: std::nothrow overload

void* buffer = malloc(sizeof(T));
T* t1 = new(buffer) T();
t1->~T(); // required: delete t1 would be wrong
free(buffer);

Outside managing memory on a rather low level as above calling destructors explicitly, however, is a sign of bad design. Probably, it is actually not just bad design but outright wrong (yes, using an explicit destructor followed by a copy constructor call in the assignment operator is a bad design and likely to be wrong).

With C++ 2011 there is another reason to use explicit destructor calls: When using generalized unions, it is necessary to explicitly destroy the current object and create a new object using placement new when changing the type of the represented object. Also, when the union is destroyed, it is necessary to explicitly call the destructor of the current object if it requires destruction.

Is it ok to use the destructor inside function of a class to reset values?

Calling a non-trivial destructor explicitly ends object's lifetime (why?).

Share the functionality between ResetStats and destructor by putting it in a separate private function:

// Both destructor and ResetStats call Reset
~Test1(){
    Reset(nullptr);
}
void ResetStats(int NewValue) {
    Reset(new int(NewValue));
}
// Shared functionality goes here
private Reset(int *ptr) {
    delete test;
    test=ptr;
    valid=false;
}

How does the compiler handle base class destructor calls in the derived destructor?

@M.M's comment hit it. You are calling the destructor twice. This is undefined behavior and anything may happen, including the behavior you observe.

(In reality, most likely one of those destructor calls modifies the vptr of the object, meaning that subsequent destructor calls no longer go to the most derived object. But that's just a guess.)

The correct thing to do is to not call the destructor manually.

Actively calling a destructor

Well, just like the process of creation of a dynamic object can be "disassembled" into two stages: raw memory allocation and actual initialization (e.g. constructor call through placement-new), the process of destroying a dynamic object can also be "disassembled" into two stages: actual de-initialization (destructor call) and raw memory deallocation. (As you can see the the two processes are mirror images of each other.)

This is very useful in situations when you want to use your own raw memory allocation/deallocation mechanism. Granted, in many cases you can achieve the desired effect by overloading operator new/delete, but in some cases it is not flexible enough and you might prefer to carry out the above steps explicitly.

So, here's one example of when the direct destructor call is a useful feature. There are quite a few others. And yes, it is perfectly legal.

When your class teacher said that you should never do that, he/she probably mean that you should avoid it for now, within the scope of your current curriculum. As you progress in your study, you will understand that many "you should never do that" tricks are actually very useful techniques belonging to "do that, if you know what you are doing" category. Of course, you should not abuse this technique, since it is indeed a low-level one.

P.S. This syntax is formally called pseudo-destructor call, since it sort of allows you to "call" non-existing destructors

typedef int INT;

INT i;
i.~INT(); // <- legal code, pseudo-destructor call, no op

The above is legal C++ code, despite the fact that INT is not a class type and therefore has no destructor. (Just don't try doing i.~int() - it is illegal. An aliased typename has to be used for non-class types.)

Is explicitly calling destructors from constructors bad practice in C++?

What's recommended way of designing this?

I would say: even more RAII. Something like:

class WSARaii
{
public:
    WSARaii()
    {
        if (WSAStartup(MAKEWORD(2, 2), &wsaData))
            throw std::runtime_error("WSAStartup function failed.");
    }
    ~WSARaii()
    {
        WSACleanup();
    }
    WSARaii(const WSARaii&) = delete;
    WSARaii& operator =(const WSARaii&) = delete;

private:
    WSADATA wsaData;
};

class Socket
{
public:
    Socket(..) : m_scListener(socket(pAddr->ai_family, pAddr->ai_socktype, pAddr->ai_protocol) {
        if (m_scListener == INVALID_SOCKET)
            throw std::runtime_error("'socket' function failed.");
    }
    ~Server() {
        if (m_scListener != INVALID_SOCKET) {
            closesocket(m_scListener);
        }
    }
private:
    SOCKET m_scListener
};

And finally

class Server {
public:
    Server() : wsa(), socket(..) {}

private:
    WSARaii wsa;
    Socket socket;
};

---

Related Topics