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.

+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. — odinthenerd, Feb 26 '14 at 09:26
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. — Janz Dott, Feb 26 '14 at 09:28
@JanzDott: That's not what "clarity" is :-S ... it's like saying that `return x` is less clear than `return x == true ? true : false`. — Kerrek SB, Feb 26 '14 at 13:57
@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. — fgrieu, Feb 26 '14 at 14:11

Kerrek SB · Accepted Answer · 2014-02-26T09:21:19.313

10

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.)

edited Feb 26 '14 at 09:21

answered Feb 26 '14 at 09:13

Kerrek SB

464,522
92
875
1,084

6

Yup; canonical useful link is [*What is the rule of three?*](http://stackoverflow.com/questions/4172722/what-is-the-rule-of-three). – Oliver Charlesworth Feb 26 '14 at 09:14
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. – Janz Dott Feb 26 '14 at 09:25
1

@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`). 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. – Kerrek SB Feb 26 '14 at 09:27
@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. – Janz Dott Feb 26 '14 at 09:52
1

@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. – Kerrek SB Feb 26 '14 at 10:10
I think SomeClass(SomeClass const & rhs) can just be SomeClass(SomeClass const & rhs) { *this = rhs; } – paulm Feb 26 '14 at 12:26
@paulm: In this particular case, it may work — if you don't forget to also initialize `this->data` to `nullptr` before assignment. – Joker_vD Feb 26 '14 at 13:16
@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. – Janz Dott Feb 26 '14 at 17:55
@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. – Kerrek SB Feb 26 '14 at 19:40

Avt · Answer 2 · 2014-02-26T09:37:10.033

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.

C++ Explanation for this delete[] Error?

2 Answers2