Sign in to follow this  
X Abstract X

Copy Constructors and Assignment Operators

Recommended Posts

X Abstract X    109
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[i].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]

Share this post


Link to post
Share on other sites
SiCrane    11839
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?

Share this post


Link to post
Share on other sites
X Abstract X    109
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?

Share this post


Link to post
Share on other sites
Wavarian    850
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]

Share this post


Link to post
Share on other sites
SiCrane    11839
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.

Share this post


Link to post
Share on other sites
jyk    2094
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.]

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this