C++ Explanation for this delete[] Error?

Question

After making a lot of changes to a project, I created an error that took me quite a while to track down.

I have a class which contains a dynamically allocated array. I then create a dynamic array of this class. I can then delete[] that array. But, if I replace an item in the array before deleting it, it causes an error. In debug mode, it gives an assertion message from dbgdel.cpp "Expression: _BLOCK_TYPE_IS_VALID(pHead->nBlockUse)". Here is a small program to demonstrate.

class SomeClass {
public:
    int *data;

    SomeClass() {
        data = nullptr;
    }
    SomeClass(int num) {
        data = new int[num];
    }
    ~SomeClass() {
        if (data != nullptr) { 
            delete[] data;
        }
    }
};

int main(int argc, char *args[]) {
    SomeClass *someArray = new SomeClass[10];

    //If you comment this out, there is no error.  Error gets thrown when deleting
    someArray[0] = SomeClass(10);

    delete[] someArray;
    return 0;
}

I'm curious, why does this happen? When the item in the array gets replaced, its destructor gets called. Then the new item allocates its data in a location separate from the array. Then delete[] calls the destructors of all the objects in the array. When the destructors get called, they should delete the item's data array. I can't imagine what the problem is, but I'd like if someone could explain.

Bernardina

+1 for creating a minimal test-case ;) – Constanta

The if (data != nullptr) is a case of zombie programming. – Kokura

+1 for asking the question in a good style. My advice is to use smart pointers, STL containers and RAII as often as possible, then you can't encounter these kinds of problems in the first place. – Babur

OliCharlesworth Yeah I assume others hate reading through irrelevant material just as much as I do :) KerrekSB I don't actually do that. I just added it for clarity. – Bernardina

@JanzDott: That's not what "clarity" is :-S ... it's like saying that return x is less clear than return x == true ? true : false. – Kokura

@PorkyBrain: I'm fine with Resource Acquisition Is Initialization. Another option is to stick to C: arguably, since it is much simpler, there's a shorter learning curse till you seldom create things that have the appearance of being correct, but are a disaster waiting to happen. – Romeoromeon

Answer 1

Your class is broken: It has a non-trivial destructor, but you do not define copy constructors and copy assignment operators. That means that the class cannot be correctly copied or assigned-to (since the destructible state is not copied or assigned appropriately), as you are noticing in your example code.

You can either make your class uncopiable (in which case your code won't compile any more), or move-only, in which case you need to define move construction and move-assignment, or properly copyable by implementing a deep copy of the data.

Here's how, add the following definitions:

Non-copyable:

SomeClass(SomeClass const &) = delete;
SomeClass & operator(SomeClass const &) = delete;

Moveable-only:

SomeClass(SomeClass const &) = delete;
SomeClass(SomeClass && rhs) : data(rhs.data) { rhs.data = nullptr; }
SomeClass & operator(SomeClass const &) = delete;
SomeClass & operator(SomeClass && rhs) {
    if (this != &rhs) { delete data; data = rhs.data; rhs.data = nullptr; }
    return *this;
}

Copyable:

SomeClass(SomeClass const & rhs) : ptr(new int[rhs->GetSizeMagically()]) {
    /* copy from rhs.data to data */
}
SomeClass & operator(SomeClass const & rhs) {
    if (this == &rhs) return *this;

    int * tmp = new int[rhs->GetSizeMagically()];
    /* copy data */
    delete data;
    data = tmp;
}
// move operations as above

The upshot is that the nature of the destructor determines the invariants of the class, because every object must be consistently destructible. From that you can infer the required semantics of the copy and move operations. (This is often called the Rule of Three or Rule of Five.)

Kokura

Yup; canonical useful link is What is the rule of three?. – Constanta

Ahh that makes sense. I never bothered with copy or assignment operators before. That's good to know. I have a lot of classes I should probably add them to. – Bernardina

@JanzDott: Probably not - the real rule of C++ is the Rule of Zero: Don't write any of this yourself, but rather factor the problem into classes with single responsibilities which take care of the resource management for you (e.g. std::vector<int>). You should essentially consider it a failure of the design process if you find yourself writing a destructor, unless you're wearing your "library maintainer" hat. – Kokura

@KerrekSB I avoid std::vector for trivial things because it is horrendously slow in debug mode. Normally that wouldn't be a problem. But it is when you're dealing with performance critical things like a 3D game engine or a ray tracer. std::vector is the reason my game engine drops from 800+ fps to a crawling lag in debug mode. That's a big problem when I'm doing profiling. – Bernardina

@JanzDott: I see. Maybe try a different compiler? GCC has an -Og mode now that's debuggable but also somewhat optimized... and the general adage is that it's easier to make correct code fast than fast code correct, and if you cannot handle the complexities of writing code "by hand" (and we've only demonstrated the most trivial kind of complexity here, which already basically killed you), then aiming for optimization is perhaps questionable. – Kokura

I think SomeClass(SomeClass const & rhs) can just be SomeClass(SomeClass const & rhs) { *this = rhs; } – Bigname

@paulm: In this particular case, it may work — if you don't forget to also initialize this->data to nullptr before assignment. – Ply

@KerrekSB Thank you for the answer but I would appreciate it if you wouldn't try to talk down to me like that. I've only been programming in C++ for around 6 months. The fact that I overlooked copy/assignment operators doesn't mean the 99% of the rest of my programming isn't good. Trivial cases don't "kill me". The kind of software I write is far from trivial. – Bernardina

@JanzDott: Well, suit yourself, but experience shows that complexity is the downfall of many projects over and over again. If you believe that you can handle it, fine, but C++ as a language was designed to give you tools to handle complexity, and I can only advise to work with the language rather than against it. It's your call, of course, and you can take my answer for the ill-informed rant that it is. – Kokura

Answer 2

Kerrek SB s answer is great. I just want to clarify that in your code memory is freed twice.

This code

someArray[0] = SomeClass(10);

is the same as this

SomeClass temp(10);
someArray[0] = temp; //here temp.data is copied to someArray[0].data

Then ~SomeClass() is called for temp and data is freed first time.

Here

delete[] someArray;

~SomeClass() is called for someArray[0] and data is freed second time.

Spielman