Destructor called twice causes crash

Question

I implemented a constructor that takes a string as argument, this causes the destructor to get called twice and then the program crashes (the program contains a raw pointer). I know that it has to do with the copy constructor that somehow gets called when this type of constructors are used. Below is a simple code that illustrates the problem.

I would appreciate your comments on how to fix the program to avoid it to crash. I need to use this kind of constructors. I would like to understand why the copy constructor gets called. I didn't do any explicit assignment.

#include <iostream>
#include <fstream>
#include <string>
using namespace std;

class DebugClass
{
public:
    DebugClass(void) {
        data = NULL;
    }

    DebugClass(std::string str) {
        data = new double[2];
        data[0] = 1.0;
        data[1] = 2.0;
    }

    DebugClass(DebugClass const& other) {
        cout << "copy construction\n"; 
    }

    ~DebugClass(void) {
        if (data)
        {
            delete [] data;
            data = NULL;
        }
    }

    double* data;
};


int main()
{
    DebugClass obj = DebugClass("Folder");
    return 0;
}

One problem is that your copy constructor doesn't copy the object. — Bo Persson, Jul 23 '15 at 14:32
What compiler are you using? I would have thought the copy in `DebugClass obj = DebugClass("Folder");` would have been elided. Using direct initialization might solve the problem, but you should define a reasonable copy constructor. Regardless, this would be avoided by avoiding raw pointers for your member data. — TartanLlama, Jul 23 '15 at 14:33
I can't see any constructor or destructor code which contains the problem. You presented code is not helpful. Can you please provide the minimum code to reproduce the failure. So please remove unused parts like `DebugClass(void);` And as a hint: Please remove all `void` pseudo arguments, especially from destructor! It looks ugly and is old c syntax. It is still allowed but not useful. — Klaus, Jul 23 '15 at 14:33
Nor does your copy-ctor member-initialize `data` (which all three of your constructors should be doing, frankly). Your default and parameterized *assign* the member, but where possible (and it is in all these cases) prefer member-initialization). — WhozCraig, Jul 23 '15 at 14:33
Related reading ["rule of 3"](http://stackoverflow.com/q/4172722/3747990) and "rule of 5" or "rule of zero". — Niall, Jul 23 '15 at 14:34
Your copy constructor generally should _copy_ the object, not just print a message. — celticminstrel, Jul 23 '15 at 14:35
I added the copy constructor to track the crash...Do I need to define a copy constructor in the example above? Thanks — user3026374, Jul 23 '15 at 14:41
For me the provided code runs without problem. I presume @TartanLlama is right that on certain architectures/compilers there is elision of the constructor. — kasterma, Jul 23 '15 at 14:41

PeterSW · Accepted Answer · 2015-07-23T15:52:16.410

6

Your copy constructor leaves data uninitialized:

DebugClass::DebugClass(DebugClass const& other) 
{ 
    cout << "copy construction\n"; 
}

So any use of data from that point on will give undefined behaviour.

If you simply set data to nullptr there, then you should not get the double deletion. Though you probably really want to do some form of copying from other.

This line:

DebugClass obj = DebugClass("Folder");

results in the copy constructor being called as part of constructing obj.

If you instead did:

DebugClass obj("Folder");

then obj would be constructed with the normal constructor.

edited Jul 23 '15 at 15:52

answered Jul 23 '15 at 14:33

PeterSW

4,921
1
24
35

If I do DebugClass obj = DebugClass("Folder"); What should I do to make it work .. It crahes even when I don't define a copy constructor – user3026374 Jul 23 '15 at 14:44
A copy constructor should make a copy of all the data in the object. In your case, you'd need to allocate a properly sized buffer assigned to `this->data` and then copy the contents of `other.data` into it. – Rob K Jul 23 '15 at 14:52
@user3026374 I would recommend finding some tutorials on C++ constructors and reading about the rule of 3, rule of 5, rule of 0. It should all be clearer have read up around those areas. – PeterSW Jul 23 '15 at 14:54

score 1 · Answer 2 · answered Jul 23 '15 at 19:26

DebugClass obj = DebugClass("Folder");

The rhs creates a nameless object and being assigned to obj which is a another new object creation, hence calling the copy constructor.

Your statement is equivalent to

DebugClass obj1;
DebugClass obj2 = obj1; // here copy-ctor gets invoked

And for the second part ...although you have define a copy-ctor, it is hardly benefits your program, because you did nothing with the pointer data member, which is left uninitialized. That leads to undefined behavior.

Though I am not aware of your intention about the class DebugClass, but in a simpler way it can be defined with another data member say array_size and you ctors can be as given below.

    DebugClass(void):array_size(0) {
    data = NULL;
}

DebugClass(std::string str, size_t aSize):array_size(aSize)
{
    if(array_size)
    {
       data = new double[array_size];
       data[0] = 1.0;
       data[1] = 2.0;
    }
}

DebugClass(DebugClass const& other):array_size(other.array_size)
{
    cout << "copy construction\n"; 
    if(array_size)
    {
       data = new double[array_size];
       for(int i=0; i<array_size; i++)
           data[i] = other.data[i];
    }
}

Hope this clarifies.

Destructor called twice causes crash

2 Answers2

Linked

Related