-2

I'm trying to implement a singly linked list on my own in C++ from scratch, having got so far with my first method append():

#include <iostream>
using namespace std;

struct ListNode{
    int key_value;
    ListNode *next;
};

class List{ 
    public:
        List();
        void append(int key);

    private:
        ListNode *head;
        ListNode *tail;
};

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

void List::append(int key){
    ListNode *newnode;
    newnode->key_value = key;
    newnode->next = NULL;

    if(head == NULL){
        head = newnode;
        tail = head;
    }
    else{
        tail->next = newnode;
        tail = newnode;
    }
    return;
}

int main(){
    try{ 
        List l1;
        l1.append(1);
        //l1.append(2);
        //l1.append(3);
        //l1.append(5);
    } 
    catch (exception const& ex) { 
        cerr << "Exception: " << ex.what() <<endl; 
        return -1;
    } 
}

It compiles without any warnings or errors, but during execution I'm only getting the message Bus error: 10. Seems there is something wrong with the way I'm initializing and using the ListNode variables and pointers, any insights would be appreciated.

Abhishek Tirkey
  • 435
  • 1
  • 6
  • 12
  • 1
    You are derefrencing `newnode` in `List::append` without first allocating a node. – drescherjm Jul 19 '16 at 21:37
  • 1
    You should consider increasing your warning level. Also after you are done understanding and correcting your code you should use `std::forward_list` or just `std::vector` instead. – nwp Jul 19 '16 at 21:42
  • Thanks, I just looked up how to allocate structures and it worked. Seems I had initially tried to fix this via a solution on another thread but didn't realize that was about allocating a class object rather than a struct. – Abhishek Tirkey Jul 19 '16 at 21:42
  • ***a class object rather than a struct.*** In c++ these are the same thing other than the default visibility. – drescherjm Jul 19 '16 at 21:43
  • Class object allocation looks like 'ListNode newnode = new ListNode()' while struct allocation looks like 'ListNode *newnode = new ListNode'. Right? – Abhishek Tirkey Jul 19 '16 at 21:46
  • No they are the same. http://stackoverflow.com/a/54596/487892 – drescherjm Jul 19 '16 at 22:12

1 Answers1

2

In this line:

ListNode *newnode;

you create uninitialized variable of pointer to ListNode and then you dereference it. Any access to unintialized variable leads to UB, but with pointers you most probably will get bus error immediatly. So allocate memory:

ListNode *newnode = new ListNode;

Note: instead of initializing data members:

newnode->key_value = key;
newnode->next = NULL;

you should provide proper constructor for ListNode:

struct ListNode {
    int key_value;
    ListNode *next;

    ListNode( int v ) : 
        key_value( v ),
        next( nullptr )
    {
    }
};

then just create it:

  ListNode *newnode = new ListNode( key );

and next 2 lines can be omitted. This will make your code cleaner and prevent from creating a ListNode instance with uninitialized data.

Note N2: as your class List has a raw pointer and ownership of the data you should follow rule of three and create or disable copy ctor, assignment operator and dtor.

Slava
  • 43,454
  • 1
  • 47
  • 90