Questions about a Segmentation Fault in C++ most likely caused by a custom copy constructor

I'm getting a segmentation fault which I believe is caused by the copy constructor. However, I can't find an example like this one anywhere online. I've read about shallow copy and deep copy but I'm not sure which category this copy would fall under. Anyone know?

MyObject::MyObject{
    lots of things including const and structs, but no pointers
}
MyObject::MyObject( const MyObject& oCopy){
    *this = oCopy;//开发者_开发百科is this deep or shallow?
}
const MyObject& MyObject::operator=(const MyObject& oRhs){
    if( this != oRhs ){
        members = oRhs.members;
        .....//there is a lot of members
    }
    return *this;
}
MyObject::~MyObject(){
    //there is nothing here
}

Code:

const MyObject * mpoOriginal;//this gets initialized in the constructor

int Main(){
    mpoOriginal = new MyObject();
    return DoSomething();
}

bool DoSomething(){
    MyObject *poCopied = new MyObject(*mpoOriginal);//the copy
    //lots of stuff going on
    delete poCopied;//this causes the crash - can't step into using GDB
    return true;
}

EDIT: Added operator= and constructor

SOLVED: Barking up the wrong tree, it ended up being a function calling delete twice on the same object

---

It is generally a bad idea to use the assignment operator like this in the copy constructor. This will default-construct all the members and then assign over them. It is much better to either just rely on the implicitly-generated copy constructor, or use the member initializer list to copy those members that need copying, and apply the appropriate initialization to the others.

Without details of the class members, it is hard to judge what is causing your segfault.

---

According to your code you're not creating the original object... you're just creating a pointer like this: const MyObject * mpoOriginal;

So the copy is using bad data into the created new object...

---

Wow....

MyObject::MyObject( const MyObject& oCopy)
{
    *this = oCopy;//is this deep or shallow?
}

It is neither. It is a call to the assignment operator.
Since you have not finished the construction of the object this is probably ill-advised (though perfectly valid). It is more traditional to define the assignment operator in terms of the copy constructor though (see copy and swap idium).

const MyObject& MyObject::operator=(const MyObject& oRhs)
{
    if( this != oRhs ){
        members = oRhs.members;
        .....//there is a lot of members
    }
    return *this;
}

Basically fine, though normally the result of assignment is not cont.
But if you do it this way you need to divide up your processing a bit to make it exception safe. It should look more like this:

const MyObject& MyObject::operator=(const MyObject& oRhs)
{
    if( this == oRhs )
    {
        return *this;
    }
    // Stage 1:
    // Copy all members of oRhs that can throw during copy into temporaries.
    // That way if they do throw you have not destroyed this obbject.

    // Stage 2:
    // Copy anything that can **not** throw from oRhs into this object
    // Use swap on the temporaries to copy them into the object in an exception sage mannor.

    // Stage 3:
    // Free any resources.
    return *this;
}

Of course there is a simpler way of doing this using copy and swap idum:

MyObject& MyObject::operator=(MyObject oRhs)  // use pass by value to get copy
{
    this.swap(oRhs);
    return *this;
}

void MyObject::swap(MyObject& oRhs)  throws()
{
    // Call swap on each member.
    return *this;
}

If there is nothing to do in the destructor don't declare it (unless it needs to be virtual).

MyObject::~MyObject(){
    //there is nothing here
}

Here you are declaring a pointer (not an object) so the constructor is not called (as pointers don;t have constructors).

const MyObject * mpoOriginal;//this gets initialized in the constructor

Here you are calling new to create the object.
Are you sure you want to do this? A dynamically allocated object must be destroyed; ostensibly via delete, but more usually in C++ you wrap pointers inside a smart pointer to make sure the owner correctly and automatically destroys the object.

int main()
{ //^^^^   Note main() has a lower case m

    mpoOriginal = new MyObject();
    return DoSomething();
}

But since you probably don't want a dynamic object. What you want is automatic object that is destroyed when it goes out of scope. Also you probably should not be using a global variable (pass it as a parameter otherwise your code is working using the side affects that are associated with global state).

int main()
{
     const MyObject  mpoOriginal;
     return DoSomething(mpoOriginal);
}

You do not need to call new to make a copy just create an object (passing the object you want to copy).

bool DoSomething(MyObject const& data)
{
    MyObject   poCopied (data);     //the copy

    //lots of stuff going on
    // No need to delete.
    // delete poCopied;//this causes the crash - can't step into using GDB
    // When it goes out of scope it is auto destroyed (as it is automatic).

    return true;
}

---

What you are doing is making your copy constructor use the assignment operator (which you don't seem to have defined). Frankly I'm surprised it compiles, but because you haven't shown all your code maybe it does.

Write you copy constructor in the normal way, and then see if you still get the same problem. If it's true what you say about 'lots of things ... but I don't see any pointers' then you should not be writing a copy constructor at all. Try just deleting it.

---

I don't have a direct answer as for what exactly causes the segfault, but conventional wisdom here is to follow the rule of three, i.e. when you find yourself needing any of copy constructor, assignment operator, or a destructor, you better implement all three of them (c++0x adds move semantics, which makes it "rule of four"?).

Then, it's usually the other way around - the copy assignment operator is implemented in terms of copy constructor - copy and swap idiom.

---

MyObject::MyObject{
    lots of things including const and structs, but no pointers
}

The difference between a shallow copy and a deep copy is only meaningful if there is a pointer to dynamic memory. If any of those member structs isn't doing a deep copy of it's pointer, then you'll have to work around that (how depends on the struct). However, if all members either don't contain pointers, or correctly do deep copies of their pointers, then the copy constructor/assignment is not the source of your problems.

---

It's either, depending on what your operator= does. That's where the magic happens; the copy constructor is merely invoking it.

If you didn't define an operator= yourself, then the compiler synthesised one for you, and it is performing a shallow copy.