CODEWITHSUNDEEP
c++linked-list
LordByron
LordByron

Reputation: 83

Simple C++ Linked List

I have plenty of previous experience with linked lists in Java, but I seem to have confused myself with this simple attempt in C++. I am getting a segmentation fault at runtime, which from what I understand has to do with assigning a null pointer, but I am at a loss for a solution.

Edit: Thank you all for the very helpful responses. The code is now working, but trying to use 

delete p;
at the end of linkedList::addNode results in a segmentation fault at runtime. Just curious if anyone knew why that is?

Here is my updated code:

#include <iostream>
using namespace std;

class Node{
    public:
        int data;
    Node * next;
    Node(int x){
        data = x;
        next = NULL;
        }
    Node(int x, Node * y){
        data = x; 
        next = y;
        }
    };


class linkedList{
Node *head;
public:
    linkedList(){
        head = NULL;
        }
    void addNode(int value){
        Node *p;
        if(head == NULL)
            head = new Node (value, NULL);
        else{
            p=head;
            while(p->next !=NULL)
                p=p->next;
            p->next = new Node (value, NULL);
            }
        }
    void print(){
        Node * p;
        p = head;
        while(p != NULL){
            cout << p->data << "\n";
            p = p->next;
            }
        }
};


int main(void){
linkedList test;
test.addNode(4);
test.addNode(76);
test.addNode(12);
test.print();
return(0);
}

Upvotes: 7

Views: 58174

Answers (9)

DeusAduro
DeusAduro

Reputation: 6066

The code is now working, but trying to use

delete p;

at the end of linkedList::addNode results in a segmentation fault at runtime. Just curious if anyone knew why that is?

Well this is an issue because the purpose of the add node function was to dynamically allocate a new node at the end of the LinkedList. So you do that correctly, now by putting 'delete p;' you are deleting the newly added node (something I imagine you don't actually want). However, to answer your question which is why this causes a segmentation fault:

You add a node, you tell head to point to this new node. Now you delete this new node without telling head that it should once again point to NULL. Thus next time you add a node, or try to print your list, it will immediately attempt to look at what head is pointing to, which is in fact released (deleted) memory, kaboom?

The correct (or at least one correct) usage of delete in your list is in a destructor, remember in C++ we always want to clean up the dynamic memory that we have allocated, your destructor might look like:

~linkedList()
{
    Node* p = head;
    while ( p!=NULL )
    {
        Node* nextNode = p->next;
        delete p;
        p = nextNode;
    }
}

By using a destructor like this you guarantee that your linkedList will be cleaned up appropriately when it goes out of scope, or is deleted.

Upvotes: 0

csj
csj

Reputation: 22416

Your delete statement is not actually doing any cleanup. By the time you call it p==null. If you want to cleanup the list, you will need to implement a separate method to iterate through, and delete each and every node.

Something like this:

void ClearList ()
{
    Node * c = head;
    Node * n;

    while (c != NULL)
    {
        n = c->next;
        delete c;
        c = n;
    }
}

Upvotes: 1

John Calsbeek
John Calsbeek

Reputation: 36497

First, in linkedList::addNode method, you have the construction if (head = NULL), which will wind up assigning to head; you want the == operator.

Second, about the line:

head = &(Node (value, NULL));

For somewhat unintuitive reasons, this won't work. You'll get a reference to a Node, but that node will go out of scope as soon as the method ends, and attempts to reference it will lead to a segmentation fault. You need to use the new operator (same with the other similar line):

head = new Node(value, NULL);

If you add a method for removing a node, make sure to delete the node then—it won't get automatically garbage-collected like it will in Java.

Sidebar: Think of what happens like this: when you do Node(value, NULL), you're using a temporary variable that's declared like this:

Node hiddenTempNode(value, NULL);

This doesn't allocate space for an object anywhere except on the stack—it's very similar to allocating space for an int and a Node * on the stack as separate variables. As a result, as soon as you leave the method, the object disappears and the pointer to it will do weird things when used.

Third, beware: you may want to set next = NULL in your single-parameter constructor, to ensure that it always has a value. Similarly for your default constructor.

Fourth: your linkedList::print method is looping until p->next is NULL and printing the value of p->next; those occurrences of p->next should probably be changed to just p if you want to get the first and last items.

Upvotes: 6

Stack Overflow is garbage
Stack Overflow is garbage

Reputation: 247969

Solution: Don't implement your own linked list. Use the one supplied by the standard library.

Upvotes: -2

Ralph
Ralph

Reputation: 5232

I'd like to add two issues that were not mentioned, yet:

  • when you 'new' objects, you must 'delete' them at some point.
  • all three of your constructors should initialize both member variables.

Upvotes: 1

jcopenha
jcopenha

Reputation: 3975

you are taking the address of variables on the stack

head = &(Node (value, NULL));

should be changed to

head = new Node(value, NULL);

same for the p->next code. Then you will want to delete these nodes in your destructor.

As for the printing try

while(p != NULL)
{
   cout << p->data << "\n";
   p = p->next;
}

Upvotes: 3

eduffy
eduffy

Reputation: 40224

For starters

if(head = NULL)

is an assignment, not a check for equality. Change it to

if(head == NULL)

Secondly,

head = &(Node (value, NULL));

Doesn't make sense* change this to

head = new Node (value, NULL);

*this actually creates a temporary object, gives you the address, then destroys that newly created object.

Thirdly,

Node(int x) { data = x; }

Leave next without a value, change this line to

Node(int x) { data = x; next = NULL; }

Upvotes: 2

anand
anand

Reputation: 11309

You are not allocating memory.You should use new to allocate it.

One more error in if(head = NULL) , it should be if(head == NULL)

void addNode(int value){
            Node *p;
            if(head == NULL)
                    head =  new Node (value, NULL);
            else{
                    p=head;
                    while(p->next !=NULL)
                            p=p->next;
                    p->next = new Node (value, NULL);
                    }
            }

Upvotes: 1

Mehrdad Afshari
Mehrdad Afshari

Reputation: 421988

You are allocating space for nodes on stack and grabbing its address, which will go away as soon as the block ends and consequently, the address will be rendered invalid. You should allocate nodes using new operator on the heap instead:

Node* node = new Node(value, NULL);

You should free everything you allocate on the heap as soon as you don't need it to prevent memory leak:

delete node;

Upvotes: 2

Related Questions