Returning a copy with overloaded operators

Question

I have a class Foo for which I've overloaded the + operator as follows:

Foo Foo::operator+(const Bar &b)
{
    Foo copy = (*this);
    if (someCondition) return copy;
    //snip
}

To me, this looks reasonable. However, whenever I am returning copy, Visual Studio alerts me of an error which 'may be due to a corruption of the heap'. Is there something wrong with what I've done?

edit: updating with more info.

The error message:

Windows has triggered a breakpoint in sample.exe.

This may be due to a corruption of the heap, which indicates a bug in sample.exe or any of the DLLs it has loaded.

This may also be due to the user pressing F12 while sample.exe has focus.

The output window may have more diagnostic information.

The copy constructor:

Foo::Foo(const Foo&p)
{
    some_pointer = p.get_some_pointer();
    some_value = p.get_some_value();
}

The code it breaks to:

//within dbgheap.c
    extern "C" _CRTIMP int __cdecl _CrtIsValidHeapPointer(
            const void * pUserData
            )
    {
            if (!pUserData)
                return FALSE;

            if (!_CrtIsValidPointer(pHdr(pUserData), sizeof(_CrtMemBlockHeader), FALSE))
                return FALSE;

            return HeapValidate( _crtheap, 0, pHdr(pUserData) );
    }

Answer 1

That type of error is usually associated with multiple deletions (or frees) of the same pointer, or with some more obscure situations (acquiring from one heap and releasing to a different heap, but that is probably not the case here).

The first thing I would do is look a the destructors and check that you are not shallow copying and doubly deleting. For example with the following code:

// buggy!!!
struct test {
    int * data;
    test() : data( new int[5] ) {}
    ~test() { delete [] data; }
    test( test const & rhs ) : data( rhs.data ) {}
    test& operator=( test const & rhs ) {
       data = rhs.data;
    }
};
int main() {
    test t1;          // 5 ints allocated int t1.data
    test t2( t1 );    // no memory allocated, t2.data == t1.data
} // t2 out of scope: t2.~test() => delete t2.data
  // t1 out of scope: t1.~test() => delete t1.data but both are the same: double delete

If this is the case, you can decide whether you want to have shallow copies or make deep copies. In the second case the copy constructor (and assignment operator) should allocate their own memory, while in the second case you must ensure that the memory is not released twice.

As always with pointers, it is better to delegate resource management to external (pre-built) classes. In the case of unique ownership (and deep copies) you should probably use std::auto_ptr (or std::unique_ptr in C++0x -- or the boost variants). In the second case, using boost::shared_ptr (or std::shared_ptr in C++0x) will ensure that the data is shared and only deleted once.