Sign in to follow this  

Copy Constructors and Assignment Operators

This topic is 2816 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

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
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
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
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
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
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

This topic is 2816 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

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