Copy Constructors and Assignment Operators

Started by
4 comments, last by Zakwayda 14 years ago
How do I properly write a copy constructor and overload the assignment operator for my class? I've shown my attempt, it compiles but I just want to make sure it does what I'm expecting. Thanks for looking. Edit: changed source code.

class A {

private:
    unsigned int nElements;
    Object* obj;

public:
    A() : nElements(0), obj(NULL) {}

    ~A() {
        delete[] obj;
        obj = NULL;
    }

    void createArray(unsigned int n) {
        nElements = n;
        obj = new Object[nElements];

        for (unsigned int i = 0; i < nElements; i++) {
            obj.initialize();
        }
    }

    A(const A& original) {
        nElements = original.nElements;
        obj = new Object[original.nElements];

        std::copy(original.obj, original.obj + original.nElements, obj);
    }

    A& operator=(const A& original) {
        nElements = original.nElements;
        obj = new Object[original.nElements];

	std::copy(original.obj, original.obj + original.nElements, obj);

        return *this;
    }

};

[Edited by - X Abstract X on March 28, 2010 5:34:13 PM]
Advertisement
No, your copy constructor and assignment operator only create one element no matter how many elements the other object contains. Also, your assignment operator leaks currently allocated memory. Is there any reason you aren't using std::vector?
Quote:Original post by SiCrane
No, your copy constructor and assignment operator only create one element no matter how many elements the other object contains. Also, your assignment operator leaks currently allocated memory. Is there any reason you aren't using std::vector?


Actually yes, I can't use std::vector for this. Thanks for pointing the flaws in my code. I've modified my source now and I believe this should work?

Your assignment operator doesn't check for the case where you might do something like this:

A a;...a = a; // Oops..


And like SiCrane said, your assignment operator will leak memory. Think about what happens to the previously stored data..

Bah I'll just make it easy for you:

const A& operator=(const A& original){	if(&original != this) // Safeguard for 'a = a' scenario	{		// Free existing data first!		delete[] obj;		// Reset object state		obj = 0;		nElements = 0;		try		{			obj = new Object[original.nElements];			std::copy(original.obj, original.obj + original.nElements, obj);			nElements = original.nElements;		}		catch(...)		{			delete[] obj;			obj = 0;			throw;		}	}	return *this;}


EDIT: SiCrane makes me cry inside..

[Edited by - Wavarian on March 28, 2010 6:56:17 PM]
No, your assignment operator still leaks currently allocated memory. Additionally your copy constructor isn't exception safe; if std::copy throws an exception, you'll leak the allocated memory.

Edit: Wavarian's assignment operator isn't exception safe either. If the new[] expression throws, the object will be left in an invalid state.
Quote:Actually yes, I can't use std::vector for this.
Homework assignment?

Your code still leaks memory. Also, you don't need to set obj to NULL in your destructor.

You might want to give this a read as well.

[Edit: Woops, beaten X 2.]

This topic is closed to new replies.

Advertisement