Segmentation fault when inserting at the end of a linked list C++

Question

Okay, so I'm trying to create a linked list of items by adding them one by one onto the end, and I also wanna print out the results.

I'm only showing part of my code (the part that I need work on), so ignore all the libraries that I'm not really making use of in this snippet:

#include <string>
#include <iostream>
#include <iomanip>
#include <cmath>
using namespace std;

struct Item {
    char letter;
    Item *next;
};

class List {
  public:
    List();
    void InsertEnd(char key);
    void Display();
    bool IsEmpty();
    void SizeOf();
  private:
    Item *head;
    Item *tail;
    int size;
};

List::List() {
    head = NULL;
    tail = NULL;
    size = 0;
}

void List::InsertEnd(char key) {
    //new item we're adding to the end
    Item* addOn = new Item();
    addOn->letter = key;
    addOn->next = NULL;

    //temporary item to traverse through list
    Item* temp = head;

    //if list is empty, head and tail both point to it
    if ( IsEmpty() ) {
        head->next = addOn;
        tail->next = addOn;
    } else {
        //once temp = tail
        if (temp->next == NULL) {
            tail->next = temp;
            temp = addOn;
        }
    }

    //update size of list
    SizeOf();

}

void List::Display() {
    cout << "Items:" << endl;
    for (Item* curr = head->next; curr != NULL; curr = curr->next) {
      cout << curr->letter << endl;
    }

cout << size << " items." << endl;

}

bool List::IsEmpty() {
    if (size == 0)
        return true;
    else 
        return false;
}

void List::SizeOf() {
    size++;
}

int main() {
    List* test = new List;

    test->InsertEnd('A');
    test->InsertEnd('B');
    test->InsertEnd('C');

    test->Display();

    return 0;
}

It compiles just fine, but when I run it, literally the only thing I get is "Segmentation fault." ???

Answer 1

I recommend that you single step in a debugger and see exactly which value is null when you crash. The debugger will show you every value at each step in your code. I've been programming for over 30 years and I single step through my code every day.

Answer 2

If the list is empty then head->next will be NULL, yet you say head->next = addOn;. Shouldn't it be head = addOn; ?

In fact this whole block of code is rubbish:

if ( IsEmpty() ) {
    // head and tail are both null so both of these lines
    // invoke undefined behavior (you cant say NULL->field = something)
    head->next = addOn;
    tail->next = addOn;
} else {
    //once temp = tail
    // temp = head. Shouldn't you just be putting "addOn" as the next item
    // in the list after tail? What if temp->next != NULL?
    if (temp->next == NULL) {
        // tail->next = head (because temp == head). That makes no sense.
        tail->next = temp;
        // So some temp variable = addOn? addOn doesn't actually go into the list?
        // Thats a memory leak right there.
        temp = addOn;
    }
}

In pseudo code what you want is something like this:

if (IsEmpty())
{
    head = addOn;
    tail = addOn;
}
else
{
    // special case when 1 item in the list (i.e. head == tail)
    if (head == tail)
    {
        // new item comes after head
        head->next = addOn;
    }
    else
    {
        // new item comes after tail
        tail->next = addOn;
    }
    // tail is now the item just added.
    tail = addOn;
}