Are there any unexpected consequences of calling a destructor from the assignment operator?

Question

For example:

class Foo : public Bar
{
    ~Foo()
    {
        // Do some complicated stuff
    }

    Foo &operator=(const Foo &rhs)
    {
        if (&rhs != this)
        {
            ~Foo(); // Is this safe?

            // Do more stuff
        }
    }
}

Are there any unexpected consequences of calling the destructor explicitly with regard to inheritance and other such things?

Is there any reason to abstract out the destructor code into a void destruct() function and call that instead?

I would say the number of cases where there are *not* unexpected consequences from doing that are very limited. Unless you expect the unexpected. — juanchopanza, Jan 23 '15 at 15:44
I'd be more inclined to provide a `clear()` method (private, if it doesn't make sense for it to be part of the public interface) and call it from both places. Even if it's not technically necessary in a particular situation, it will look a lot less weird and is *substantially* less likely to trip up the next person who has to maintain your code. — cdhowie, Jan 23 '15 at 15:46
Is it that youre worried about freeing up resources prior to assigning new values? If so, I would rather use the compare-swap idiom or write a cleanup function — excalibur, Jan 23 '15 at 16:22

score 5 · Accepted Answer · answered Jan 23 '15 at 15:54

5

Calling the destructor is a bad idea in the simplest case, and a horrible one the moment the code gets slightly more complex.

The simplest case would be this:

class Something {
public:
    Something(const Something& o);
    ~Something();
    Something& operator =(const Something& o) {
        if (this != &o) {
            // this looks like a simple way of implementing assignment
            this->~Something();
            new (this) Something(o); // invoke copy constructor
        }
        return *this;
    }
};

This is a bad idea. If the copy constructor throws, you're left with raw memory - there isn't an object there. Only, outside the assignment operator, nobody notices.

If inheritance comes into play, things become worse. Let's say Something is in fact a base class with a virtual destructor. The derived class's functions are all implemented with a default. In this case the derived class's assignment operator will do the following:

It will call its own base version (your assignment operator).
Your assignment operator calls the destructor. This is a virtual call.
The derived destructor destroys the members of the derived class.
The base destructor destroys the members of the base class.
Your assignment operator calls the copy constructor. This isn't virtual; it actually constructs a base object. (If the base class isn't abstract. If it is, the code won't compile.) You have now replaced the derived object with a base object.
The copy constructor constructs the base's members.
The derived assignment operator does a memberwise assignment of the derived class's members. Which have been destroyed and not re-created.

At this point, you have multiple instances of UB heaped upon each other, in a glorious pile of complete chaos.

So yeah. Don't do that.

answered Jan 23 '15 at 15:54

Sebastian Redl

69,373
8
123
157

Just to be clear, I was never planning on invoking the copy constructor as well. Also, if the copy constructor throws, wouldn't the assignment operator just pass the throw on? – Matt Jan 23 '15 at 16:16
@Matt That just makes it worse, because then you have just raw memory after the assignment even in the non-error case. Just to be clear: calling the destructor ends the lifetime of the object, logically. Any further operation on the object is undefined behavior, because there's no object there anymore. The only thing you can do with that memory (and must do, because the compiler probably will still call the destructor implicitly at some point) is use placement new to create a new object. – Sebastian Redl Jan 23 '15 at 16:24
@Matt As for throwing on, yes it will. What's your point? If you catch the exception outside of the assignment, you still have some raw memory hiding under the dead husk of the poor object you destroyed. – Sebastian Redl Jan 23 '15 at 16:25
Well then I guess I'd have to catch it an fix it in the assignment operator itself. No different from any other time a placement new throws. It still has nothing to do with my question about the destructor. – Matt Jan 23 '15 at 16:48
@Matt What if the default constructor throws too? How do you fix it then? It has everything to do with the destructor, because I'm telling you why you shouldn't call that, ever. – Sebastian Redl Jan 23 '15 at 20:26
Where am I calling the default constructor? I didn't mean that your answer has nothing to do with the destructor, but that all your arguments about the constructor have nothing to do with the destructor. Unless all you're saying is that if I fail to construct, I will not be able to do something like `this = 0`, which I would be able to do if I were outside the object. – Matt Jan 23 '15 at 21:20
I'm saying that if you destruct you *have* to construct immediately afterwards, or you will get UB. But the constructor has to actually finish successfully, so in order to do this, you need some constructor that is guaranteed not to throw. – Sebastian Redl Jan 23 '15 at 22:01

score 0 · Answer 2 · answered Jan 23 '15 at 15:57

0

Absolutely not.

void f()
{
    Foo foo, fee;
    foo = fee; <-- a first foo.~Foo will be called here !
} <-- a second foo.~Foo will be called here and fee.~Foo as well !

As you can see you have 3 calls to destructor instead of expected 2 calls.

You should NOT use a *self-*destructor inside a constructor or a non-static method.

answered Jan 23 '15 at 15:57

hlide

237
2
10

That second `foo.~Foo` will be safe because the object would have been reconstructed immediately after being destructed the first time. Also, the way I write my code, calling the destructor twice is safe anyway. – Matt Jan 23 '15 at 16:12
Formally, it's still undefined behaviour, AFAIK. Anyhow, if reconstruction throws an exception, you are SOL, then you have a destroyed object with all the nasty consequences. Create a temporary and with that instead, i.e. delay destroying the actual payload of `this` until you can guarantee exception safety. – Ulrich Eckhardt Jan 23 '15 at 16:15
@UlrichEckhardt, I'm the one reconstructing it, so I can handle any exception. – Matt Jan 23 '15 at 16:24
still, there are a lot of constraints to watch like not sharing true pointers. Doing so might lead to some nasty bugs, especially for other people who may replace you in the development of a product and are not clever enough not to mess up with. – hlide Jan 25 '15 at 22:57

Are there any unexpected consequences of calling a destructor from the assignment operator?

2 Answers2

Linked