asked on

C++, copy constructor

Dear experts, please have a look at the code below:

#include <iostream>

using namespace std;

class A{
private:
            int valueInt;
      public:
            void print();
            void printObject(A objectA);
            void setValue(int value);

            A (A& arg){
                  cout<<"copying constructor: "<<arg.valueInt<<endl;
                  arg.valueInt = 1;
            }

            A (int x);

};

void A:: print(){
      cout<<this->valueInt<<endl;
}

A::A(int x){
      this->valueInt = x;
}

void A::printObject(A objectA){
      objectA.print();

}

void A::setValue(int value){
      this->valueInt = value;
}

int main()
{
      A c(1);
      A d(2);

      c = d; //I would expect copying constructor being called here but it is not. Why?

      c.printObject(d); //it calls copying constructor which is fine.
//what I do not understand is: why does it print random values?

      return 0;

}

Thank you

panJames

ASKER CERTIFIED SOLUTION

evilrix

membership

This solution is only available to members.

To access this solution, you must be a member of Experts Exchange.

Start Free Trial

SOLUTION

farzanj

membership

This solution is only available to members.

To access this solution, you must be a member of Experts Exchange.

Start Free Trial

panJames

ASKER

"You should be assigning to "this" not to "arg".

My way of thinking:

A (A& arg){
cout<<"copying constructor: "<<arg.valueInt<<endl;
arg.valueInt = 1;

}

arg is passed by reference here so according to my understanding 'arg.valueInt = 1;' should have assigned this value to arg which in our case is object 'd' and also argument for
"void A::printObject(A objectA){". Where do I get it wrong?

panJames

ASKER

Ok, I got it now!

evilrix

>> Where do I get it wrong?
I'm sure you've figured it out but basically you are performing your initialisation in the wrong direction... you should be initialising "this" with the values from arg and not the other way around.

Also, not really important to this specific question but, you really should use constructor initialisation lists rather than in-constructor assignment. The former (unfortunately) has a slightly uglier syntax but it is the correct way to construct a class. Although the latter works it is very inefficient because you are constructing class members and then assigning to them. You are doing two jobs where only one is required. There are also times (const members, for example) where in-class assignment just won't work (you won't be able to compile the code).

In your case this is what your copy-constructor should look like...

A (A const & arg) : valueInt(arg.valueInt) {
cout<<"copying constructor: "<<arg.valueInt<<endl;
}

http://www.learncpp.com/cpp-tutorial/101-constructor-initialization-lists/
http://www.cprogramming.com/tutorial/initialization-lists-c++.html

Note, how this also makes it clearer which way round the initialisation is performed?

Finally, since your class doesn't contain any reference or pointers and all its members are, in fact, copyable, you don't really need a copy-constructor - the one synthersized by the compiler would work just fine (which performs a shallow rather than deep copy). If this is just an exercise to learn about copy-constructors then do ignore this point, but just bare it in mind.