0

I was trying to understand Copy constructor and Operator. I read some code but I just didn't get it.

Here is main function

int main()
{
Cwin win1('A',"window")
Cwin win2;
win1 = win2;
win1.set_data('B',"hello");
win1.show();
win2.show();
}

I extract the most important code from Class Cwin

Class Cwin
{
  private:
  char id, *title ;

  public:
  Cwin(char i, char *text){......} //constructor
  Cwin(){......}  //default constructor, id ='D' , *title = "default"
  Cwin(const Cwin &win)
  {
    id = win.id;
    strcpy(title,win.title);
  }
  ......
};

Output :

B : hello
B : hello

I can understand what causes it. But I can't understand below solution that fix it.

Class Cwin 
{
   private:
   char id, *title ;

   public:
   Cwin(char i, char *text){......} //constructor
   Cwin(){......}  //default constructor, id ='D' , *title = "default"
   Cwin(const Cwin &win)
   {
     id = win.id;
     strcpy(title,win.title);
    }

   void operator=(const Cwin &win)
   {
     id = win.id;      
     strcpy (this->title , win.title);
   }
   ......


 };

Output:

B : hello
D : default

Why change it strcpy (title , win.title); into strcpy (this->title , win.title); makes a huge difference?

Christophe
  • 68,716
  • 7
  • 72
  • 138
rj487
  • 4,476
  • 6
  • 47
  • 88
  • Changing from `strcpy (title , win.title)` to `strcpy (this->title , win.title)` doesn't make a difference, unless there is a variable named `title` which is local to the function. http://stackoverflow.com/help/mcve – Benjamin Lindley Jun 04 '15 at 18:02
  • it wrote an operator to overload `=`, does it cause this result? – rj487 Jun 04 '15 at 18:07
  • @CodaChang I don't understand what you're asking, you defined an assignment operator and it's not behaving the way you want it to? What behavior did you expect instead? Your copy constructor seems to be copying to an uninitialized pointer (`title`), which is undefined behavior. Please edit the question to indicate exactly what the problem is, and, as Benjamin says, add an MCVE. – Praetorian Jun 04 '15 at 18:09
  • Yes, probably. Since without that, the default assignment operator just copies the pointer, which means your objects will point to the same data, and changing the contents of the string that one points to will change the other as well. I have a feeling your code has much more serious problems though. But since you've abbreviated it so heavily, I can't tell what's actually your real code, and what's just short-hand for what you're really doing. Which is why I asked for an [MCVE](http://stackoverflow.com/help/mcve) – Benjamin Lindley Jun 04 '15 at 18:10
  • thx for your advice, and I have understand this issue. I will update my question. – rj487 Jun 05 '15 at 15:55

3 Answers3

3

Explanation:

Inside of a member function, you can write this->title or title. Both are equivalent, unless you have a local function variable called title which name would then hide the member.

WHat makes the difference is that you have added an assignement operator.

Without assignement operator your the statement win1=win2 proceeds by copying member by member. In this situation, a pointer is copied as is, so that after the copy, both objects will point to the same pointed char*:

enter image description here

When you then set_data(), the string is copied to the char* pointer by win1, but win2 points to the same value.

Your second code snippet, handles the copy much better: the content of the string pointed to is copied/duplicated to char* pointed by the other object. win1 and win2 will hence continue to use 2 distinct strings.

Remarks/advices

  • You use strcpy() without checking that the target is long enough to contain the copied string. This might result in buffer overflow, memory corruption, and a lot of other horrible things.

  • I'd strongly advise that you'll use std::string instead of char* whenever possible: The default copy would have been well managed, and you don't have to care for memory allocation/deallocation,length.

Community
  • 1
  • 1
Christophe
  • 68,716
  • 7
  • 72
  • 138
  • 1
    +1. I also promot using std::wstring instead of regular strings. people tend to underate the fact that not all people communicate with english. better be prepared from the start. – David Haim Jun 04 '15 at 22:28
  • 1
    @DavidHaim: `wchar_t` is not portable across platforms, and that affects `std::wstring`. If you need to support multiple platforms, either use `std::u16string` for UTF-16 strings, or use `std::string` to hold UTF-8 strings. – Remy Lebeau Jun 04 '15 at 22:39
  • 1
    Thanks, it's a great explanation and advice. Actually, I met the bug you point out, and I fix the bug by editing `char` to `wstring` – rj487 Jun 05 '15 at 16:10
2

If Cwin does not explicitly implement its own assignment-operator (which your first example does not), the compiler will generate a default implementation that simply copies member values as-is from one object to another. That is why your output said the same thing for both objects - the win1 = win2 statement using the default assigment-operator implementation merely overwrote the pointer value of win1.title with the pointer value of win2.title, and thus both title members were left pointing at the same memory block, which the subsequent win1.set_data() statement populated with data.

To do what you are asking, you should change title to use std::string instead of char*, let it handle the copying for you. It will work fine with the compiler's default copy-constructor and assignment-operator implementations (unless you have other data that you need to make manual copies of):

#include <string>

Class Cwin
{
private:
  char id;
  std::string title;

public:
  Cwin()
    : id('D'), title("default") {}

  Cwin(char i, const std::string &text)
    : id(i), title(text) {}

  void set_data(char i, const std::string &text)
  {
    id = i;
    title = text;
  }

  void show()
  {
    std::cout << id << " : " << title << std::endl;
  }
};

But, if you need to use char* directly, you must implement the copy-constructor and assignment-operator properly to ensure your title data gets copied correctly, eg:

#include <cstring> 

Class Cwin
{
private:
  char id, *title ;

public:
  Cwin()
    : id(0), title(NULL)
  {
    set_data('D', "default");
  }

  Cwin(char i, char *text)
    : id(0), title(NULL)
  {
    set_data(i, text);
  }

  Cwin(const Cwin &win)
    : id(0), title(NULL)
  {
    set_data(win.id, win.title);
  }

  ~Cwin()
  {
    delete[] title;
  }

  Cwin& operator=(const Cwin &win)
  {
    if (this != &win)
      set_data(win.id, win.title);
    return *this;
  }

  void set_data(char i, char *text)
  {
    int len = std::strlen(text);
    char *newtitle = new char[len+1];
    std::strcpy(newtitle, text);

    delete[] title;
    title = newtitle;
    id = i;
  }

  void show()
  {
    std::cout << id << " : " << title << std::endl;
  }
};

Any time you have to manually allocate memory in a constructor and free it in a destructor, chances are good that you will also need to copy that data in a copy-constructor and assignment-operator as well. Read up about the The rule of three/five/zero. In this situation, CWin needed to follow the Rule of three in order to function properly with char*, but can follow the Rule of zero when using std::string. You should always strive to write code that follows the Rule of zero whenever possible, it makes things easier to manage.

Remy Lebeau
  • 555,201
  • 31
  • 458
  • 770
  • 1
    Thanks , so I need to remember this concept. As I see pointer, I need to implement its copy constructor. You have been a great help. – rj487 Jun 05 '15 at 16:13
  • Read up about [The rule of three/five/zero](http://en.cppreference.com/w/cpp/language/rule_of_three). – Remy Lebeau Jun 05 '15 at 16:55
2

C++ classes have a default assignment operator, that does a shallow copy whenever you assign one member of that class to other member unless you overload it. (except at the time of initialization, where it uses the copy constructor )

Now, coming to your code in the first part there is no overloading of the assignment operator, therefore what it does is do the shallow copy of the elements of win2 to win1 which in turn copies the 'id' and the 'title' pointer(not the string what it is pointing to(storing)). The main function will run as follows :

int main()
{
  Cwin win1('A',"window")
  /*
    This will create a new object win1
    win1.id = 'A' and win1.title will point to "window"
  */
  Cwin win2;
  /*
    This will create a new object win2
    win2.id = 'D' and win2.title will point to "default"
  */

  win1 = win2;
  /*
  This will do somewhat the following
  win1.id = win2.id;
  win1.title = win2.title; ( note that  the pointer is copied )
  Now , win1.id = 'D' win2.id = 'D'
  win1.title points "Defalult"
  win2.title points "Default"
  */

  win1.set_data('B',"hello");
  /*
  This sets the win.id = 'B'
  and now the win1.title points to "hello"
  Also since win1.title and win2.title are both the same pointer
  so win2.title will also point to  "hello"
  */
  win1.show();
  win2.show();
}

But in the second part you have overloaded the assignment operator which instead of copying the pointer copies the string it is pointing to(storing).The main will now run as follows :

int main()
{
  Cwin win1('A',"window")
  /*
    This will create a new object win1
    win1.id = 'A' and win1.title will point to "window"
  */
  Cwin win2;
  /*
    This will create a new object win2
    win2.id = 'D' and win2.title will point to "default"
  */

  win1 = win2;
  /*
  This will now do what is in the overloaded assignment operator
  where to copy the string strcpy is used which will copy the content
  of the string instead of copying the pointers
  Now , win1.id = 'D' win2.id = 'D'
  win1.title points "Defalult"
  win2.title points "Default"
  */

  win1.set_data('B',"hello");
  /*
  This sets the win.id = 'B'
  and now the win1.title points to "hello"
  win2.title will still point to  "Default"
  */
  win1.show();
  win2.show();
}

Hence , the given results. And you must also see and follow the advice given in this answer.

Community
  • 1
  • 1
Vishwas
  • 506
  • 1
  • 5
  • 20
  • Thanks for your explanation, I read this answer and your description, now I understand this issue. You have been a great help :) – rj487 Jun 05 '15 at 15:59