0

I'm trying to write a class Set with linked list to store integers. After compiling and running on my Mac's terminal, this is the output:

[]
[10]
[10, 20]
Segmentation fault: 11

But I was expecting to see the following output:

[]
[10]
[10, 20]
[10, 20]
[10, 20, 30]

I'm wondering if it is a problem with my operator= function, or is it that I cannot use the operator= function with pointers? If this is so, how should I correct the problem so that the program outputs as I expected? I would really appreciate all your help. Thanks in advance!

#include <iostream>
using namespace std;

class Node {
  public:
    int value;
    Node* next;
    Node(int n, Node* ptr = NULL) : value(n), next(ptr) {}
};

class Set {
  Node* head;
  friend ostream& operator<<(ostream&, const Set&);
  public:
    Set() : head(NULL) {}
    Set(const Set& another){ *this = another; }
    ~Set();
    Set& operator+=(const int&);
    Set& operator=(const Set&);
};

int main() {
    int num1 = 10;
    int num2 = 20;
    int num3 = 30;
    Set set1;
    cout << set1;
    Set* set2;
    set1 += num1;
    cout << set1;
    set1 += num2;
    cout << set1;
    set2 = new Set(set1);
    cout << *set2;
    *set2 += num3;
    cout << *set2;
    delete set2;
    return 0;
}

Set::~Set() {
    Node* current = head;
    while (current != NULL) {
        Node* temp = current;
        current = current->next;
        delete temp;
    }
}

Set& Set::operator+=(const int& aNum) {
    if (head == NULL) {
        head = new Node(aNum);
        return *this;
    }
    Node* previous = head;
    Node* current = head->next;
    while (current != NULL) {
        previous = current;
        current = current->next;
    }
    previous->next = new Node(aNum);
    return *this;
}

Set& Set::operator=(const Set& another) {
    if (this != &another) {
        Node* current = head;
        while (current != NULL) {
            Node* temp = current;
            current = current->next;
            delete temp;
        }
        Node* anotherCurrent = another.head;
        while (anotherCurrent != NULL) {
            *this += anotherCurrent->value;
            anotherCurrent = anotherCurrent->next;
        }
    }
    return *this;
}

ostream& operator<<(ostream& os, const Set& s) {
    os << "[";
    for (Node* p = s.head; p != NULL; p = p->next) {
        os << p->value;
        if (p->next != NULL)
            os << ", ";
    }
    os << "]" << endl;
    return os;
}
πάντα ῥεῖ
  • 1
  • 13
  • 116
  • 190
mlkw
  • 71
  • 5
  • 4
    The right tool to solve such problems is your debugger. You should step through your code line-by-line *before* asking on Stack Overflow. For more help, please read [How to debug small programs (by Eric Lippert)](https://ericlippert.com/2014/03/05/how-to-debug-small-programs/). At a minimum, you should \[edit] your question to include a [Minimal, Complete, and Verifiable](http://stackoverflow.com/help/mcve) example that reproduces your problem, along with the observations you made in the debugger. – πάντα ῥεῖ Nov 04 '16 at 16:52
  • `Set(const Set& another){ *this = another; }` -- This is not a good way to write a copy constructor. You should write the copy constructor without any help from the assignment operator (pretend the assignment op doesn't exist). Then once you do that, writing the assignment operator becomes a 2 line function. – PaulMcKenzie Nov 04 '16 at 17:28
  • @PaulMcKenzie thx for your help. I have rewritten the copy constructor without using the assignment operator and it worked. However, I don't know why the copy constructor cannot use the assignment operator? Also, how should the assignment operator be written using the copy constructor? Thanks so much ^ – mlkw Nov 05 '16 at 02:44
  • @πάνταῥεῖ thx for your reply. I have no problem running the code on Xcode, the problem only exists when I run it in terminal. Also, I was actually working on a much larger project with several more header files. Before posting, I actually wrote the code from scratch to include only the necessary functions to produce the same output, and function that I think is causing the problem. The main() I provided is also just the examples to test the functions. Thx anyways and I'll be sure to bookmark the links you provided :) – mlkw Nov 05 '16 at 02:57
  • 1
    @mlkw - The assignment operator: `{ Set temp(another); std::swap(temp.head, head); return *this; }` That is all you need to do if you write your copy constructor independent of the assignment operator, and make sure your Set destructor works properly. Simple enough? Basically when you implement the rule of 3, start with copy constructor and destructor first -- leave the assignment operator for last as it becomes trivial. – PaulMcKenzie Nov 05 '16 at 03:13

1 Answers1

3

you have to set head to NULL when deleting your previous list prior to copying, or else the += operator will use head and it is currently unallocated but not NULL

Set& Set::operator=(const Set& another) {
    if (this != &another) {
        Node* current = head;
        while (current != NULL) {
            Node* temp = current;
            current = current->next;
            delete temp;
        }
        head = NULL;   // <============== code to add
        Node* anotherCurrent = another.head;
        while (anotherCurrent != NULL) {
            *this += anotherCurrent->value;
            anotherCurrent = anotherCurrent->next;
        }
    }
    return *this;

BTW a very interesting design pattern, a must read: copy-and-swap idiom

Community
  • 1
  • 1
Jean-François Fabre
  • 137,073
  • 23
  • 153
  • 219
  • Thanks a lot for your help. I added the line of code but the problem still persists. However, rewriting the copy constructor without using the assignment operator did the trick although I have no idea why. Thanks anyways and I'll be sure to read the link you provided xd – mlkw Nov 05 '16 at 02:39
  • @mlkw - Read my comment to you. The assignment operator becomes a 3 line function once you changed the copy constructor (all due to using the copy / swap idiom). – PaulMcKenzie Nov 05 '16 at 03:18