Overriding the operator= in C++

Question

My task is to implement a method that overrides the = operator. I have something written, but honestly I have no idea what I'm doing. Can someone explain what the point of doing this overriding is (not in general, I understand that, but just in this case, Im a bit confused)? What is my method supposed to accomplish? Where does my logic fail in my current code?

scene.cpp:70: error: no match for ‘operator=’ in ‘*(((Image*)(((long unsigned int)i) * 80ul)) + newArray) = *(((Scene*)this)->Scene::images + ((Image**)(((long unsigned int)i) * 8ul)))’

Answer 1

It is very tricky to implement a copy constructor or assignment operator in a class that manages memory, and keep your code exception safe. Exception safety means ensuring that if an exception is thrown at any point in your program, that your manually managed memory is still cleaned up correctly. A few idioms have cropped up to help with this:

• RAII

RAII's basic tenant is that if you have to manage memory (which much more often lately, you don't), wrap the memory handling in an object that allocates only one piece of memory on construction, and deallocates that memory on destruction.

• Copy-and-Swap

Copy and swap is specific guidance for implementing a non-trivial assignment operator when you have to deal with memory allocation. This is the exact scenario you are trying to solve.

I recommend you read about both quite a bit before continuing to try to write code that manages memory, or you'll probably tear your hair out trying to squash all your bugs.

An alternative to implementing idioms yourself is to rely on code like std::vector and tr1::shared_ptr to do your memory management for you. Plenty of people who know C++ and memory management oddities inside-out use both of those on a regular basis. You can very often get away with just those.

Answer 2

What your (inner) code does in English:

//allocate new memory and copy
images = new Image*[source.maximum];

This sets images to a newly allocated array of source.maximum uninitialized Image pointers. Whatever images was pointing to is lost.

    Scene(source);

This creates a new temporary Scene object and then throws it away. it does not "re-call" the constructor on this.

    //deallocate old memory
    delete *source;

This, if it worked, would dereference source (which is a const Scene&, so this only works if T* Scene::operator *(void) is defined, where T is some type) and delete the pointed-to T object.

    //assign
    source=images;

This tries to copy images over source, which shouldn't happen since source is const. Once created, a reference cannot be changed to reference a different object.

    this->maximum=images.maximum;

This doesn't work. images is a Image** which does not have a maximum field. Also, the this-> is redundant.

UPDATE: Regarding then new version:

First, you don't need to say this-> everywhere.

for (int i=0;i<source.maximum;i++)
    this->images[i]=source->images[i];

The problem here is that source is a reference, not a pointer, so you should be using source.images[i] instead of source->images[i].

Assuming this is fixed, now the issue is that the image objects are pointed to by both the current object as well as source. If either object releases memory (delete images[i]; images[i] = 0; or similar), then the pointer in the other object becomes invalid. If images are never deleted, then this code is fine.

If, however, you want this object to have its own copies of the images, then you need to do more work:

if(this != &source)
{
    // Delete old images.
    for(int i = 0; i < maximum; i++)
        delete images[i];
    delete[] images;

    // Copy new images.
    maximum = source.maximum;
    images = new Image*[maximum];
    for(int i = 0; i < maximum; i++)
        images[i] = new Image(*(source.images[i]));
}

This assumes you have a copy constructor for Image (Image::Image(const Image&);).

Finally, Scene should have a copy constructor that works similarly to this one, except it doesn't need to delete old stuff. If you don't make copies of images, use:

Scene::Scene(const Scene& original): maximum(original.maximum)
{
    images = new Image*[maximum];
    for(size_t i = 0; i < maximum; i++)
        images[i] = source.images[i];
}

If you do make copies of images, use:

Scene::Scene(const Scene& original): maximum(original.maximum)
{
    images = new Image*[maximum];
    for(size_t i = 0; i < maximum; i++)
        images[i] = new Image(*(source.images[i]));
}

In both cases, don't forget to add Scene(const Scene& original); to the class definition.

Answer 3

Overriding an operater is like overloading a function now; you can do a copy via the = op.

Scene A;
Scene B;

A.do_things();
B = A;

If you didn't overload the = operator; B=A would have made B a pointer to the same object as A (since class instance variables in C are pointers).

Answer 4

Not quite sure what you are wanting it to do. But its got lots of faults

Typically, operator= would take a copy of the original without making any changes to the original.

This looks like you are wanting to transfer ownership of your images from one object to another.

delete *source is just DANGEROUS as it isn't / shouldn't be the owner. source could easily be a stack variable.

source != images so assigning images to source is just going to kill things

storing the images in a vector would handle allocating memory and also copying of all the images for you.

Answer 5

There are a few things wrong with your code.

1. Do not delete source, you pass it in as a reference to const, so leave it alone!
2. Do not alter source. It is const so you can't.
3. Do no allocate new memory to this->images whithout a delete[] images first.
4. You need to understand the difference between references and pointers first.
5. You need to understand what const is.
6. You cannot assign images to source for a variety of reasons. One being that you shouldn't (can't) assign to source. Another is that, unless a Scene is an Image, you cannot go assigning an Image to a Scene pointer.

Answer 6

There's a canonical solution to implementing a non-trivial assignment operator:

Scene const & Scene::operator=(Scene s) { swap(s); return *this; }
void Scene::swap(Scene & s) /* nothrow */ {
    Image *temp_images = images; images = s.images; s.images = temp_images;
    int temp_maximum = maximum; maximum = s.maximum; s.maximum = temp_maximum;
}

This is completely exception-safe, as it relies on the copy-constructor to create a temporary copy of the source before swapping it in (using a no-throw swap) to replace the target. If any part of the copy goes awry, both source and target will remain as before.

The only thing it doesn't do is optimise self-assignment. It does, however, work correctly in the presence of self-assignment, and I don't generally bother optimising something that shouldn't happen (and which rarely does, in practice).

Another option is to use a more conventional parameter type:

Scene const & Scene::operator=(Scene const & s) {
    Scene temp(s);
    swap(temp);
    return *this;
}

However, apart from being more wordy, this version is potentially less efficient, since it guarantees that a copy is made, whereas the pass-by-value version may elide the copy if the parameter passed to s is an rvalue.