Why is this destructor being called immediately after creation?

Question

I have the following class:

class FixedByteStream {
public:
FixedByteStream() : size(0), address(NULL), existing(false) {}
FixedByteStream(int length) : existing(false) {
    size = length;
    address = new char[length];
}
FixedByteStream(int length, char* addr) : existing(true) {
    size = length;
    address = addr;
}
FixedByteStream(string* str, bool includeNull = false) : existing(true) {
    size = (*str).length();
    address = const_cast<char*>((*str).c_str());
    if (includeNull){
        ++size;
    }
}
~FixedByteStream() {
    if (existing == false) {
        delete [] address;
    }
    address = NULL;
}
int getLength() {
    return size;
}
char* getAddressAt(int index) {
    return &(address[index]);
}


char& operator[] (const int index) {
    return address[index];
}
operator char*() {
    return address;
}

private:
    bool existing;
    int size;
    char* address;
};

And a very simple test that is able to produce the issue:

FixedByteStream received;
received = FixedByteStream(12);
received[0] = 't';

Valgrind warns about an invalid write, and debugging has shown why. FixedByteStream received; calls the constructor with no arguments (which is kind of stupid because it can't do anything). received = FixedByteStream(12); calls the constructor with the integer argument... and then immediately calls the destructor on itself, invalidating the object. It still works for some reason, but I'd rather it not be placed in such an odd predicament that throws warnings.

So, why is it being called there? I could somewhat understand if the destructor were called first, to get rid of the useless temporary object (not that it needs to), but I have used that kind of declare-now-assign-later pattern practically everywhere and never had such an issue before.

Answer 1

You are missing an assignment operator. Remember the rule of three (or five).

The problem is roughly like this:

T t; // default constructed t
t = T(2); // T(2) constructor with a single argument, assignment operator= called with this == &t

You provide no assignment operator so the pointer value in the temporary is simply copied into t and then the memory pointed to is deleted in the destructor of the temporary.

Also: Don't have a default constructor, if the object constructed is invalid.

Answer 2

If you object has any user defined constructor it is always constructed using a constructor. Just defining an object without any constructor arguments uses the default constructor independent of whether the object is being overwritten afterwards. That is

FixedByteStream received;

will call the default constructor. The next line is a bit more interesting:

received = FixedByteStream(12);

This line create a temporary FixedByteStream with the argument 12. Internally this will allocate some memory but since temporaries are destroyed at the end of the full expression (in this case essentially when the semicolon is reached) that won't you do much good. Once this temporary object is constructed it is assigned to received using the automatically generated copy assignment which would look something like this if you'd write it manually:

FixedByteStream& FixedByteStream::operator= (FixedByteStream& other) {
    this->existing = other.existing;
    this->size     = other.size;
    this->address  = other.address;
    return *this;
}

That is, once this assignment is executed, you have to identical copies of FixedByteStream, one of which is about to be destroyed and will release the resources just allocated. This is clearly not what you want, i.e. you definitely need to implement the copy assignment operator to make your class well-behaved. In general, the presence of a destructor which does anything interesting is a good hint at the fact that you'll need an assignment operator as well. In fact, there is another one of the generated operations, namely the copy constructor, which does roughly what the copy assignment does except that it copy constructs the members instead of assigning them. This is also not what you want with your class.

Now the interesting question becomes: how can FixedByteStream be fixed? Effectively, you'll need either to use reference counting to keep track of how many objects are currently looking at the FixedByteStream, allocate a copy of the content, or you need to use move semantic support (aka rvalue references) which is only available in C++2011. Unless you really know what you are doing I'd recommend copying the stream in all cases and leave a more advanced approach for later.

Answer 3

Step by step:

//create a new object using the default constructor
//I don't see why you think it's stupid that the constructor is called
//it's doing what you're telling it to do
FixedByteStream received;

//FixedByteStream(12) creates a temporary object
//you then copy this object in the received object you already have
//you're using the default operator =
//the temporary object is deleted after it is copied to received
received = FixedByteStream(12);

//received is a valid object
received[0] = 't';

EDIT:

As I see there's a lot of wrong answers for this question, I'll explain further. I might get some hate for this, but this is a pretty important concept and I downvoted so that a wrong answer doesn't get accepted and taken from granted.

You're basically initializing some object on the stack.

I'll simplify your case:

class A
{
    A() {}
    A(const A& other) {}
    A& operator = (const A& other) {}
};

Let's talk about scope:

{ //begin scope
  A a;  //object a is created here
        //default constructor is called
} //a is destroyed here
  //destructor is called

So far so good.

Now, assignment:

{
   //two objects are created with default constructor
   A a;
   A b;
   //object b is assigned to a
   //operator = will be called
   //both objects are still alive here
   a = b;
   //...
} // a and b will be destroyed, destructor called

On to the last part:

{
   A a;
   a = A();
}

is pretty much equivalent to:

{
   A a;
   {
      A b;
      a = b;
   }
}

When you call a = A(), A() creates a temporary object which is assigned to a and then destroyed.

So object b in my simplification is the temporary object that gets destroyed. Not a, your original, therefore a is still valid.

Not the assignment operator declaration. If you don't define one, a default is used. You probably want to write your own in this case.

Answer 4

FixedByteStream(12) is assigned to received through the default operator=. Notice that you didn't allocate FixedByteStream(12) in the heap by using new, but rather allocated it in local scope without specifying a name for the variable that would hold it.

Your code is somewhat similar to:

FixedByteStream received;
FixedByteStream temp(12);
received = temp;
received[0] = 't';

Only in my example temp has a name and its scope is the entire function, and in your test code temp doesn't have a name, it exists for one line only and then gets destroyed.

The FixedByteStream(12) object you've created cannot be used after this line because it's not even a named varaible.

Answer 5

Object is already initialized on this line

FixedByteStream received;

On line

received = FixedByteStream(12);

You reinitialize it. The correct way to do this is:

FixedByteStream received(12);
// Or
FixedByteStream *received;
received = new FixedByteStream(12);

(I'd definitely go for first one)

Answer 6

It seems you don't understand object lifecycle and mistakenly interpret this code as a Java code.

When you write FixedByteStream received; an object of type FixedByteStream is created using the no-argument constructor. And when you write received = FixedByteStream(12); another object is created, = operator is called and the newly created object is desctructed.

Also you didn't override operator= so the object is copied by bytes, which is not correct.