Clobbering automatic implicit objects

Question

Let's assume the following scenario. I have an implicitly shared class as defined below:

class MyClass
{
public:
    MyClass (void) {
        data = new Data();
        data->refs = 1;
        data->dummy = 32;
    }

    ~MyClass (void) {
        if (--data->refs == 0)
            delete data;
    }

    MyClass (const MyClass& c) {
        ++(data = c.data)->refs;
    }

    MyClass& operator = (const MyClass& c) {
        ++(data = c.data)->refs;
        return *this;
    }

private:
    struct Data {
        int refs;
        int dummy;
    } *data;
};

The idea is that when this class gets copied, the internal pointer to the data gets copied and the number of references to that data is incremented. However, consider the following:

int main (void)
{
    MyClass c1;
    c1 = MyClass();
    c1 = MyClass();
    return 0;
}

My understanding is that there are three instances of MyClass and only the last instance gets deallocated. If this is the case, what can I do to avoid these scenarios and ensure that every instance of MyClass gets cleaned up?

Have you considered [`std::shared_ptr`](http://en.cppreference.com/w/cpp/memory/shared_ptr) ? — Borgleader, Sep 13 '13 at 23:48
Follow the [Rule of Three](http://stackoverflow.com/questions/4172722/what-is-the-rule-of-three) and you'll be fine. — Oliver Charlesworth, Sep 13 '13 at 23:49
One thing your not doing at all is protecting against self-overwrite. Well, you are, but only because your assignment operator is utterly broken. It is effectively abandoning your previously constructed data block before assuming a reference with the rhs. IN other words, this leaks on assignment. Use `std::shared_ptr<>`. It was made for things like this. — WhozCraig, Sep 13 '13 at 23:59
All three instances of MyClass get deallocated -- they are all on the stack, and the C++ compiler handles deallocation automatically for stack objects. — Jeremy Friesner, Sep 14 '13 at 00:20

score 3 · Accepted Answer · answered Sep 13 '13 at 23:55

In your assignment operator you need to make sure that the assigned to object is cleaned up before overwriting its value by a new value. After all, this left hand side of the assignment already references a value. The easiest way to write an assignment operator is to leverage the existing copy constructor and destructor and use a function swap() to exchange the two objects:

MyClass& MyClass::operator = (MyClass c) {
    this->swap(c);
    return *this;
}

void MyClass::swap(MyClass& other) {
    std::swap(this->data, other.data);
}

This way the value is already copied in the value c and the current value is exchanged with the one held by c. When c gets destroyed the reference count gets decremented as needed.

Note, that std::share_ptr<T> already does reference counting using a rather fancy mechanism: you might want to use the standard class rather than rolling your own.

score 2 · Answer 2 · answered Sep 13 '13 at 23:54

2

MyClass& operator = (const MyClass& c) {
    ++(data = c.data)->refs;
    return *this;
}

This is broken. Before you clobber this.data, you need to decrement its reference and delete it if the reference drops to zero.

answered Sep 13 '13 at 23:54

David Schwartz

179,497
17
214
278

Clobbering automatic implicit objects

2 Answers2