C++ pointer to class, call function where class-instance is freed, is that good practice?

Question

I have a class, let's call it A. There are two subclasses of class A which are a and b.

I'm making a pointer of class A like this:

A *pointer;

At some point in the program I initialize the pointer like this:

pointer = new a();

At some other point, I run a function of class A:

pointer->function(&pointer);

This function is inside class A (so all subclasses have it). There is a chance that when this function is called, I want to change pointer to another subclass, here is what I tried:

void A::function(A **pointer)
{
    if (something)
    {
        delete *pointer;
        *pointer = new b();
    }
}

Although this works, I'm really curious if this is good practice, I'm calling delete from inside the object and freeing the object itself, could this be undefined behavior and I got lucky it worked? Am I not understanding this right? Am I making this more complicated than it should be?

Answer 1

Yes, that's valid as long as you are careful. See more discussion at a question specifically about delete this.

However, as with other things in C++ which are valid as long as you are careful, you are better to find another solution that will be less prone to errors. I suggest you reworking you code into a function returning a new pointer, and having the old one automatically destroyed (via a smart pointer, for example).

Something along the lines:

struct A {
    static std::shared_ptr<A> function(std::shared_ptr<A>& ptr, int x) {
        if (x > 0)
            return std::make_shared<A>(x);
        else return ptr;
    }

    A(int _x): x(_x) {}

    int x;
};

Note also I made function() to be static, as it anyway accepts the object as its first argument. See live on coliru.

In fact, I don't quite like this solution with shared_ptr, and if someone will give a better implementation of this approach, I'll be glad to know.

Answer 2

This code is valid ( for more information about correctness see this answer ).
But it's not a good practice, because other developer can miss nuance, that using one of member functions will lead to reconstruction of object.
It's better to explicitly reconstruct object than hide it in member function. Or just use smart pointers.

Answer 3

As a design I don't like that a pointer suddenly points to another object (of a different type) when it is not clear that it happens. It could be argued that since OPs code passes &pointer it indicates that it may change. However, I prefer an assignment instead - I think that is more clear.

I would try something like this:

int uglyGlobal = 1;  // don't try this at home...  ;-)

class A
{
public:
    int n;
    A() {n = uglyGlobal++; cout << "A cons for #" << n << endl;}
    virtual ~A() {cout << "A des for #" << n << endl;}
    unique_ptr<A> function(int something, unique_ptr<A> ptr);
};

class a : public A
{
public:
    a() {cout << "a cons" << endl;}
    ~a() override {cout << "a des" << endl;}
};
class b : public A
{
public:
    b() {cout << "b cons" << endl;}
    ~b() override {cout << "b des" << endl;}
};

unique_ptr<A> A::function(int something, unique_ptr<A> ptr)
{
    if (something == 0)
    {
        // Turn it into an A
        return unique_ptr<A>(new A);
    }
    else if (something == 1)
    {
        // Turn it into an a
        return unique_ptr<A>(new a);
    }
    else if (something == 2)
    {
        // Turn it into an b
        return unique_ptr<A>(new b);
    }
    else
        // Keep the current
        return ptr;
}

int main()
{
    cout << "Make A" << endl;
    unique_ptr<A> x (new A);

    cout << "1. call - turn A into a" << endl;
    x = x->function(1, move(x));

    cout << "2. call - turn a into b" << endl;
    x = x->function(2, move(x));

    cout << "3. call - turn b into another b" << endl;
    x = x->function(2, move(x));

    cout << "4. call - keep current b" << endl;
    x = x->function(3, move(x));

    cout << "Return from main" << endl;
    return 0;
}

The output is:

Make A
A cons for #1
1. call - turn A into a
A cons for #2
a cons
A des for #1
2. call - turn a into b
A cons for #3
b cons
a des
A des for #2
3. call - turn b into another b
A cons for #4
b cons
b des
A des for #3
4. call - keep current b
Return from main
b des
A des for #4

Answer 4

In general, You must ensure that every execution path that call function doesn't have a stack frame for a method of A or derived class (this is invalid). So, it is dangerous. In MFC api programming, this happens "normally" in the WM_NCDESTROY message handler. In it you do thing like delete this, but Windows ensure that WM_NCDESTROY is the last message sent to a window.

I suggest you to change a little the api of class A and use unique_ptr to handle the memory:

#include <memory>

class A
{

public:

    std::unique_ptr<A> f()
    {
        return std::make_unique<A>();
    }
};

int main()
{
    auto p =  std::make_unique<A>();
    p = std::move(p->f());
    return 0;
}

In this way, you move the destruction from inside f() to the assignment of p.