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

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 2

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.