C++

Forum

 
Deep copy with =operator

techjohnny (32)
Is this how my operator overload class should look?
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
 
  SortListClass& SortListClass::operator=(const SortListClass& rhs)
{

	// Similar to Copy Constructor, except
	// - Avoid self-assignments such as  “X = X;”
	// - Delete existing this-instance content before 
	//   making this-instance a copy of the rhs instance

	if (this == &rhs)
	{		return *this;	}
	delete[] this;
	head->item = rhs.head->item;

}

private:
	struct SortListNode
	{
		SortListItemType  item;
		SortListNode     *next;
	};

	int       size;  // number of items in list
	

	SortListNode *head;  // pointer to linked list of items
	SortListNode *last;
	SortListNode *ptrTo(int position) const;
	// Returns a pointer to the node at position (1 .. k) in list
};
jonnin (11436)
looks correct, have you tested it?
there are probably ways to break something due to that delete, but you would have to contrive something odd.

what happens if you have

SortListClass doh;
SortListClass doh2;

doh = doh2;

This is probably way outside of intended use, but people do stuff.
Last edited on
mbozzi (3932)
Line 9:
delete this[];
Is extremely suspect in that it assumes that *this was allocated with a new[] expression. If it is not, your code will exhibits undefined behavior. It is generally a good idea to avoid committing suicide (delete this, or explicitly calling your own destructor), although there are cases when it makes sense.

So:

You should release the memory resources owned by *this, not *this itself. Those are the linked-list nodes, implying you should walk the list and delete each node individually. If such code exists in the destructor (it should), you can simply call it:
this->~SortListClass();
And then reuse storage, copy-constructing a new object in your place from the right-hand side:
new (this) SortListClass(rhs);

This forwards the work of copying over to the copy-constructor. You should post your copy-constructor.

Note:
In modern C++ it is relatively unusual that you need to implement the functions described in the Rule of Three (or five, if you prefer). The Rule of Three says that if you implement one of the copy-constructor, copy-assignment operator, or destructor, you should implement them all.

The purpose of following the Rule of Three is to support RAII. The purpose of RAII is to reduce the risk of resource leaks, even in the presence of exceptions.

The design I just discussed is an indication of poor (especially exception-unsafe) design. In fact, even the two lines of code I have posted are terribly broken: the copy-construction can fail -- if that happens, you're in trouble, since your data was just destroyed.

The alternative to this awfully ugly code above is the well-known copy-and-swap idiom. Post your copy constructor and I'll go into more detail.
Last edited on
techjohnny (32)
Okay, my first example did not make sense. I am trying to do a deep copy. This seems to work:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
 

// assignment operator: Make DEEP copy
SortListClass& SortListClass::operator=(const SortListClass& rhs)
{
	// TODO
	// Similar to Copy Constructor, except
	// - Avoid self-assignments such as  “X = X;”
	// - Delete existing this-instance content before 
	//   making this-instance a copy of the rhs instance

	if (this != &rhs)
	{
		delete[] head;
		head = rhs.head;
	}
	return *this;
	

}


my header file:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
 
private:
	struct SortListNode
	{
		SortListItemType  item;
		SortListNode     *next;
	};

	int       size;  // number of items in list
	

	SortListNode *head;  // pointer to linked list of items
	SortListNode *last;
	SortListNode *ptrTo(int position) const;
	// Returns a pointer to the node at position (1 .. k) in list
};


Now the following seems to copy and print correct:
1
2
3
4
5
 
SortListClass newlist3;
	newlist3.operator=(newlist2);
	
	newlist3.PrintsortList();

mbozzi (3932)
Line 14:

That's not a deep copy. Simple pointer assignment "shares" the resource between the left and right-hand sides of the assignment expression.
techjohnny (32)
Okay, I'm a little confused. I'm a little confused on the left side what is suppose to be deleted? Is it item that should receive delete [] item, but that's not a pointer that can be deleted, so shouldn't it by delete [] head;?

Do I need to first create a new heap ?

The below works. 
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
 
SortListClass& SortListClass::operator=(const SortListClass& rhs)
{
	// TODO
	// Similar to Copy Constructor, except
	// - Avoid self-assignments such as  “X = X;”
	// - Delete existing this-instance content before 
	//   making this-instance a copy of the rhs instance
	SortListClass copyme;
	if (this != &rhs)
	{
		delete[] copyme.head;
		copyme.head = rhs.head;
	}
	return *this;


}

 
 
newlist.operator=(newlist3);

Each newlist and newlist3 seem to occupy different areas of memories, since newlist.remove(1) does not affect newlist3.remove(1);
newbieg (764)
I think GFreak's example here might help you
http://www.cplusplus.com/forum/general/68198/
coder777 (8444)
The below works.
Certainly not.

copyme is a local variable the is destroyed at the end of the function. So basically you do nothing.

You indeed need to delete the entire list. delete[] head; would only work if SortListNode has a destructor which deletes its member next.

What you need to do is: for each node of rhs create/add a node and copy item.
techjohnny (32)
Okay, still a little confused on what to delete. Do I need to create a new item on the heap and access -> item ?
Cubbi (4774)
techjohnny wrote:
still a little confused on what to delete. Do I need to create a new item on the heap

For copy assignment of a linked list, you really should only delete the nodes at the end of your list if the list in rhs is shorter than yours, and create new nodes at the end of your list if the list in rhs is longer than yours.. or just do as mbozzi suggested and reuse your destructor (not by calling any sort of delete!) and your copy constructor, either directly or via copy-and-swap - it's just three lines of code either way.

techjohnny (32)
Okay, so now using:

 
 
head->item = rhs.head->item;



This is printing correctly in my main program.

What should my delete [] function look like? I am still not sure what to delete, but the assignment says "// - Delete existing this-instance content before
// making this-instance a copy of the rhs instance "

I have tried
 
 
delete [] this

but that crashes my program.
mbozzi (3932)
Your class owns a pointer to some memory somewhere (i.e., head).
This memory is a "resource".

When you simply copy the pointer, both objects get a pointer to the exact same resource. That's a shallow copy. A deep copy creates a copy of the resource.

Take the left-hand-side's resource -- e.g., head and everything head owns nodes (next, and so on), and delete them all. Freeing everything the list owns is the job of the destructor, so this code belongs there.
Something like
1
2
3
4
5
6
7
 
~List() { // destroy every node.
  // Assumes each node was allocated with a independent new-expression
  for (Node *it = head, *next; it; it = next) 
    next = it->next;
    delete it;
  }
}

Then you can call your destructor to release the stuff you own, and use the copy-constructor to replace your stuff with deep copies of the right-hand-side's stuff.

Assuming a correct copy-constructor, this will leave you with a complete copy-assignment operator that looks like this: 
1
2
3
4
5
6
7
 
SortListClass& operator=(SortListClass const& rhs) {
  if (std::addressof(rhs) == this) return *this; // careful for self-assignment
  this->~SortListClass(); 
  new (this) SortListClass(rhs);
  // Edit: need to return something :)
  return *this
}


This is still subject to the same issues that I mentioned above (it's exception-unsafe), but it's a start.
Last edited on
gunnerfunner (2127)
OP: I've tried to implement a combination of max and cubbi's suggestions for the copy ctor and copy assignment particularly (a) making deep copies through the copy ctor; (b) calling the dtor within copy assignment and (c) using the copy-swap idiom:
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
 
#include<iostream>
#include <utility>//std::swap moved from <algorithm> to <utility> in C++11

struct Node
{
	Node* next;
	int data;
	Node(){}
	Node(const int d) : next(nullptr), data(d)  {}
	~Node() {std::cout << "Goodbye " << data << "\n";}//vocal dtor
};
struct LList
{
    Node* start = nullptr;

    LList(){} //default ctor;
    LList(const LList& rhs);//copy ctor
    LList& operator = (LList rhs);//copy assignment operator
    ~LList();//dtor

    void insert (const int& item);
    void display()const;
};

int main()
{
	LList n{};
	n.insert(1);
	n.insert(2);
	n.insert(3);

	LList s{};
	s.insert(3);
	s.insert(4);
	s.insert(5);
	s.insert(7);

	std::cout << "List s before assignment \n";
	s.display();
	s = n;

	std::cout << "\nlist s after assignment \n";
	s.display();

	LList p{s};
	std::cout << "Diplaying list p \n";
	p.display();

	std::cout << "Displaying list n \n";
	n.display();
}
LList::LList(const LList& rhs)
{
    Node dummy{0};
    for (Node* o = rhs.start, *n = &dummy; o; o = o -> next)
    {
        n -> next = new Node (o -> data);
        n = n -> next;
    }
    start = dummy.next;
    std::cout << "\n";
}
LList& LList::operator = (LList rhs)//copy assignment operator
{
        if(start)
        {
           this -> ~LList();
        }
        std::swap(rhs.start, start);
        rhs.start = nullptr;
        return *this;
}
LList::~LList()
{
	    Node* temp = start;
	    while (temp)
        {
            Node* next = temp -> next;
            delete temp;
            temp = next;
        }
        start = nullptr;
}
void LList::insert(const int& item)
{
	Node* temp = new Node(item);
	if(!start)
    {
        start = temp;
    }
    else
    {
        Node* last = start;
        while (last -> next)
        {
            last = last -> next;
        }
        last -> next = temp;
    }
}
void LList::display()const
{
	Node* show = start;
	while (show)
	{
		std:: cout << show ->data << "\n";
		show = show->next;
     }
}
/*Program output: 
List s before assignment
3
4
5
7

Goodbye 3//s not empty, so dtor called during assignment
Goodbye 4
Goodbye 5
Goodbye 7

list s after assignment
1
2
3

Goodbye 0//Node dummy within copy ctor
Diplaying list p
1
2
3

Displaying list n//n has been assigned to s, hence empty

Goodbye 1//lists s and p being deleted
Goodbye 2
Goodbye 3

Goodbye 1
Goodbye 2
Goodbye 3*/


Last edited on
mbozzi (3932)
Line 63 has a little typo:
LList& LList::operator = (LList/* & */rhs)//copy assignment operator
gunnerfunner (2127)
thanks for the spot, edited above
Topic archived. No new replies allowed.