-2

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

lightswitch05
  • 8,149
  • 6
  • 45
  • 71
  • 3
    What does the `operator=(const MyObject&)` look like? – Sjoerd Aug 25 '11 at 16:29
  • 4
    Can you provide a *complete* compilable example that demonstrates the issue? Eliminate the "lots of stuff going on" if need be, it may or may not be relevant to the problem. – Oliver Charlesworth Aug 25 '11 at 16:29
  • Who keeps upvoting sub-standard questions? I see it all the time. Is somebody trying the fast-track to badges, or what? – Lightness Races in Orbit Aug 25 '11 at 16:43
  • I've added the overator=, as much as I would like to just post all the code I don't think my employer would be happy. Plus its like 5,000 lines – lightswitch05 Aug 25 '11 at 16:54
  • 1
    5,000 lines for `operator=`! Good grief. That's your problem right there. But to answer your original question, your copy constructor has the semantics of your assignment operator, so to tell whether it's shallow or deep you need to look at the assignment operator code. But I don't think this is your real problem, the real problem is that you have an unmanageable mess for a class. – john Aug 25 '11 at 17:00

7 Answers7

2

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.

Anthony Williams
  • 62,015
  • 12
  • 122
  • 149
  • Sorry, There is a operator= in the code, I had somehow overlooked it. I've added it to the pseudo code – lightswitch05 Aug 25 '11 at 16:50
  • The outline `operator=` you've posted suggests memberwise assignment, in which case the default assignment operator would work fine, and avoid the need for these 5000 lines. However, the details depends on the types of the members, and whether there are any members that are omitted from the list, but shouldn't be. – Anthony Williams Aug 26 '11 at 06:48
1

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

Pedro Ferreira
  • 734
  • 7
  • 19
  • 1
    The comment in the OP's code snippet says "this gets initialized in the constructor"... – Oliver Charlesworth Aug 25 '11 at 16:33
  • @Oli: Since he passes `*mpoOriginal` for construction, the only way that can work is through the copy constructor. But there's nothing to copy! This looks like the directly correct answer to me (+1), though there are plenty of other things wrong with this code, and it would _still_ be nice to actually see valid code (instead of "stuff here, it works I promise!"). – Lightness Races in Orbit Aug 25 '11 at 16:40
  • @Oli: but that comment is pretty ambiguous. And there's enough stuff elided from the example that it's hard to know what is OK or not in the OP's code. All we really know is that the OP is calling the assignment copy operator in the copy-ctor. Pretty much everything else is guesswork. – Michael Burr Aug 25 '11 at 16:42
  • He may be initialising `*mpoOriginal`'s members, but he's almost certainly not allocating any space for them. – Lightness Races in Orbit Aug 25 '11 at 16:46
  • `mpoOriginal` is initialized to a `new MyObject` in `Main()` – Anthony Williams Aug 26 '11 at 06:47
0

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.

john
  • 71,156
  • 4
  • 49
  • 68
0

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.

Community
  • 1
  • 1
Nikolai Fetissov
  • 77,392
  • 11
  • 105
  • 164
0
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.

Mooing Duck
  • 56,371
  • 16
  • 89
  • 146
0

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;
}
Martin York
  • 234,851
  • 74
  • 306
  • 532
-1

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.

Lightness Races in Orbit
  • 358,771
  • 68
  • 593
  • 989
  • 2
    But, if he's correct when he says he has const members, then the compiler could not have synthesised an assignment operator. That's what is confusing me. – john Aug 25 '11 at 16:33
  • According to the code given, MyObject's destructor doesn't free any pointer memebrs. If that's true, it's more likely that the '//lots of stuff going on' bit invalidates the poCopied pointer somehow. – AndrzejJ Aug 25 '11 at 16:36
  • @john: Where does he say that he has `const` members? – Lightness Races in Orbit Aug 25 '11 at 16:38
  • @Tomalak: Well he doesn't say it exactly, I was interpreting his words in the above code 'lots of things including const - but I don't see any pointers'. Of course I could be wrong. – john Aug 25 '11 at 16:40
  • @john: That's in the ctor body not member-initialiser, so I don't think he means members. Then again, his ctor doesn't even have an argument list. It's impossible for either of us to know for sure until he actually shows us some C++. – Lightness Races in Orbit Aug 25 '11 at 16:42
  • An op= has been added to the question it seems. – Lightness Races in Orbit Sep 01 '11 at 09:15