Issue in Deep Copy Constructor

Question

Please have a look at the following code

Location.h

#pragma once
class Location
{
public:
    Location(void);
    Location(int,int,int);
    ~Location(void);
    Location(const Location *loc);

    void display();
    void set(int,int,int);

private:

    int x,y,z;
};

Location.cpp

#include "Location.h"
#include <iostream>
#include <string>

using namespace std;


Location::Location()
{
}

Location::Location(int x,int y, int z)
{
    set(x,y,z);
}



Location::~Location(void)
{
}


void Location::display()
{
    cout << "X: " << x << endl;
    cout << "Y: " << y << endl;
    cout << "Z: " << z << endl;
}

void Location::set(int xx,int yy,int zz)
{
    x = xx;
    y = yy;
    z = zz;
}

Object.h

#pragma once
#include "Location.h"

class GameObject
{
public: 
    GameObject(int);
    ~GameObject(void);
    GameObject(int,Location *);
    GameObject(const GameObject *obj);

    Location *location;

    int getNumberOfObjects();

    static int counter;


    int id;

private:
    GameObject(void);



};

Object.cpp

#include "GameObject.h"
#include <iostream>

using namespace std;

static int counter = 0;

int GameObject::counter = 0;

GameObject::GameObject(void)
{
}

GameObject::GameObject(int i)
{
    counter++;
    id = i;
}

GameObject::GameObject(int i,Location *loc)
{
    id = i;
    location = loc;
}

GameObject::GameObject(const GameObject *ob)
{
    this->location = new Location(ob->location);
}


GameObject::~GameObject(void)
{
}

Main.cpp

#include <iostream>
#include "GameObject.h"

using namespace std;

int main()
{
    //GameObject obj1;
    //cout << obj1.id << endl;

    GameObject obj2(45);
    cout << obj2.id << endl;;
//  cout << obj2.counter << endl; 

    //Declaring dynamic objects for Location
    Location *loc1 = new Location(1,1,1);
    Location *loc2 = new Location(2,2,2);
    Location *loc3 = new Location(3,3,3);


    //Assigning each GameObject a location
    GameObject obj3(45,loc1);
    GameObject obj4(45,loc2);
    GameObject obj5(45,loc3);

    //Displaying Number of GameObject objects
    cout << "Number of Objects: " << GameObject::counter << endl;

    //Invoking Location's display() method using GameObject objects
    cout << "......obj3 values........" << endl;
    obj3.location->display();
    cout << "......obj4 values........" << endl;
    obj4.location->display();
    cout << "......obj5 values........" << endl;
    obj5.location->display();


    //Declaring new Static GameObject
    GameObject obj6(obj4);

    //Invoking display() member function using both obj4 and obj6 GameObjects
    cout << endl;
    cout << "Invoking display() member function using both obj4 and obj6 GameObjects " << endl;
    obj4.location->display();
    obj6.location->display();

    //Changing the location values in obj4
    obj4.location->set(8,8,8);

    //Invoking display() member function using both obj4 and obj6 GameObjects
    cout << endl;
    cout << "Invoking display() member function using both obj4 and obj6 GameObjects " << endl;
    obj4.location->display();
    obj6.location->display();




    system("pause");
    return 0;
}

In here, as you can see in Main.cpp, obj6 'Location' has been changed as soon as obj4 'Location' is changed. I don't want to change the obj6 'Location' when I change the location for obj4.

I get this error when this is executed

GameObject.obj : error LNK2019: unresolved external symbol "public: __thiscall Location::Location(class Location const *)" (??0Location@@QAE@PBV0@@Z) referenced in function "public: __thiscall GameObject::GameObject(class GameObject const *)" (??0GameObject@@QAE@PBV0@@Z)

I tried to do it with a deep copy constructor, but it didn't work. Please help.

score 4 · Accepted Answer · edited May 23 '17 at 12:29

4

GameObject::GameObject(const GameObject *ob)

is not the copy constructor, You need:

GameObject::GameObject(const GameObject &ob)
                                       ^^^

Also, you need to provide a copy assignment operator which does a deep copy.

Note that the ideal solution is to use smart pointer as member instead of an raw pointer.
This shall basically save you all the hassles of deep copying.

Good Read:
What is The Rule of Three?

edited May 23 '17 at 12:29

Community

1
1

answered Dec 06 '12 at 05:57

Alok Save

202,538
53
430
533

Actually, if he does a deep copy, isn't it very likely he wants a Location which is initialized to being equal (copy), and not another way to reference the same Location (pointer)? – Agentlien Dec 06 '12 at 06:24
@Agentlien: Yes he needs a copy to a `Location` which is similar to the `Location` object member of the class from which he copies. Note *similar* not *same*. On a side note,I did not completely understand your comment. – Alok Save Dec 06 '12 at 06:29
What I meant was, his usage scenario makes it look like what he wants is for a GameObject to always have a Location. So, no reason to hold it dynamically at all. Instead of using a smart pointer, he should just have a Location member. – Agentlien Dec 06 '12 at 06:33
@Agentlien: Perfect. I did not happen to understand OPs logic closely but surely if one can use a object member rather than a pointer member, the former is always prefereable. – Alok Save Dec 06 '12 at 06:35
Understandable. :) Well, I've added my own answer to that effect. – Agentlien Dec 06 '12 at 06:40
@Agentlien: And Sire that gets my +1. – Alok Save Dec 06 '12 at 06:52

score 3 · Answer 2 · answered Dec 06 '12 at 05:56

3

tl;dr Try implementing a copy constructor instead of a conversion cnstructor, i.e.:

//this
GameObject(const GameObject &obj);
//instead of this
GameObject(const GameObject *obj);

answered Dec 06 '12 at 05:56

Luchian Grigore

253,575
64
457
625

Thanks a lot for replying :) I really appreciate it :) +1 from me – PeakGen Dec 06 '12 at 18:31

score 2 · Answer 3 · answered Dec 06 '12 at 06:40

First, as others have pointed out, you have not implemented a copy constructor.

GameObject::GameObject(const GameObject *ob)

This is a function which takes a pointer to a const GameObject. A copy constructor should take a const reference to the object it is to be created from:

GameObject::GameObject(const GameObject &ob)

However, I see another problem as well. If you truly do want a deep copy, that means you want the Location objects to be separate and that modifying one shouldn't affect the other. Given this, I see no reason why GameObject should hold a pointer to a Location, rather than a simple Location member. Thus, I'd suggest to change this, and make a copy constructor which uses the initializer list to make your location a copy of the given Location:

Put this in your GameObject header:

class GameObject
{
public: 
    GameObject(int);
    ~GameObject(void);
    GameObject(int,const Location&);
    GameObject(const GameObject &obj);

    Location location;

    int getNumberOfObjects();

    static int counter;


    int id;

private:
    GameObject(void);
}

And write the constructors like this:

GameObject(int i)
: id(i)
{}

// This will cause GameObject to be uninitialized
~GameObject(void)
{}

GameObject(int i,const Location& loc)
: id(i), location(loc)
{}

GameObject(const GameObject &obj)
: id(obj.id), location(obj.location)
{}

Finally, you need to use the same logic to construct a proper Location copy constructor with the signature:

Location(const Location &loc);

I'll leave that implementation as an exercise to the reader. ;)

Also, reading some of your comments (like the "static" comment above a member which is actually not static, etc.) makes me think you may want to read a bit more about memory management in C++. :) Google up a bit on "C++ free store". A tip is not to use dynamic memory where it is not necessary. In C++, not every object should be created with new.

Thanks a lot for replying :) I really appreciate it :) Special thnk goes to giving me an idea about doing it in another way. Thanks a lot :) +1 from me — PeakGen, Dec 06 '12 at 18:31

score 2 · Answer 4 · edited Dec 06 '12 at 07:23

2

In C++, objects can be created on the heap and their memory address is stored in a pointer. Objects can also be created on the stack and can be indicated by a reference.

When defining our copy constructor, we need to use reference instead of pointer. The reason is that using pointer in the copy constructor causes you to have two pointers pointing to the same object in the heap. So your "obj6" and "obj4" pointer to the same object in memory.

edited Dec 06 '12 at 07:23

glglgl

89,107
13
149
217

answered Dec 06 '12 at 06:56

Jerry Chen

31
1

Thanks a lot for replying :) I really appreciate it :) +1 from me – PeakGen Dec 06 '12 at 18:32

score 1 · Answer 5 · answered Dec 06 '12 at 06:32

1

I see you don't have an implementation for the copy contructor for the Location object. I guess the problem will get solved as soon as you implement the method Location(const Location* loc).

Location(const Location* loc) {
   this->x = loc->x;
   this->y = loc->y;
   this->z = loc->z;
}

answered Dec 06 '12 at 06:32

user1880062

85
1
5

Thanks a lot for replying :) I really appreciate it :) +1 from me – PeakGen Dec 06 '12 at 18:32

Issue in Deep Copy Constructor

5 Answers5