Reseting object m_object = Object(new, parameters);

Question

I have a simple class:

class Histogram {
    int m_width;
    int m_height;
    int m_sampleSize;
    int m_bufferWidth;
    int m_bufferHeight;

    uint8* m_buffer;
    int m_size;
public:
    Histogram() : m_buffer(0) { }
    Histogram(int width, int height, int sampleSize) {
        m_buffer = new unsigned char [width*height*sampleSize];
    }
    ~Histogram() {
        my_log("destructor: buffer: %p", m_buffer);
        if ( m_buffer ) { delete [] m_buffer; m_buffer = NULL; }
    }
    unsigned char* buffer() {
        return m_buffer;
    }
};

It is a member in other class:

class Other {
    Histogram m_histogram;

    void reset() {
        my_log("reset() called: buffer: %p", m_histogram.buffer());
        m_histogram = Histogram(512, 512, 2);
    }
}

Now, I first create "uninitialized" object using Histogram() constructor – which sets m_buffer to NULL;

Then, I call the reset method, which does m_histogram = Histogram( 512, 512, 3 ) – the new object has m_buffer initialized via new.

So expected sequence of log messages is:

"reset() called: buffer: 0x0"
"destructor: buffer: 0x0"

But instead, I get:

"reset() called: buffer: 0x0"
"destructor: buffer: 0x072a7de"

So some irrational action is being performed. Moreover, the 0x072a7de address is displayied when I also delete the second object (created with "larger" constructor, with three int parameters).

The default copy constructor and assignment operator generated by the compiler does not behave nicely with pointers. You need to obey the `rule of three` (or five in C++11) see: http://stackoverflow.com/a/255744/14065 — Martin York, Jul 20 '12 at 07:05

score 2 · Answer 1 · answered Jul 20 '12 at 06:56

You MUST realize copy-ctor and assignment operator for your class Histogram, since

m_histogram = Histogram(512, 512, 2);

is assignment operator call. Implicit operator = bitwise copy members of your class.

And you must use delete[] in destructor, not delete, since you allocate an array.

juanchopanza · Accepted Answer · 2012-07-20T07:00:11.123

1

First of all, since you are pointing to a dynamically allocated array, you need to use operator delete[]

delete[] m_buffer;

Second, and more importantly, since you have dynamically allocated memory, you should follow the rule of three and implement a copy constructor, and assignment operator, as well as fixing the destructor.

What happens now is that your (compiler synthesized) assignment operator is making a "shallow" copy, i.e. it is copying the pointer. Then you will hve multiple destructors trying to delete it. You are invoking undefined behaviour.

You could really save yourself a lot of trouble by using an std::vector<uint8> as a buffer.

edited Jul 20 '12 at 07:00

answered Jul 20 '12 at 06:54

juanchopanza

223,364
34
402
480

Sorry, in oryginal code I did use delete[]. As for the rest, I thought that following sequence will occur: a) delete old m_histogram, b) construct new Histogram(512, ...), c) copy it to space used by m_histogram. It could also create new histogram in the old space directly. Your answer states, that a) is done as last step? – GameTCoder Jul 20 '12 at 07:06
@GameTCoder the RHS of your expression creates the new Histogram, but the assignment operator is used to assign to the LHS. This just makes a shallow copy of the RHS's pointer. The RHS is a temporary and gets deleted, so you are left with a dangling pointer in the LHS. – juanchopanza Jul 20 '12 at 07:11
Thanks! This explains the unexpected behavior. – GameTCoder Jul 20 '12 at 07:13

Reseting object m_object = Object(new, parameters);

2 Answers2