0

I'm trying to make a linked list and attach its nodes as such:

    Node *a,*b,*c,*d,*e,*f,*g = new Node();
    a->data=1;
    a->next=b;

    b->data=2;
    b->next=c;

This gives me a segfault at a->data = 1 What is wrong with this approach? The full code is here

user248884
  • 851
  • 1
  • 11
  • 21
  • 2
    `a` is not initialized, you only allocate memory for `g`. –  Jan 29 '18 at 17:59
  • 1
    _"What is wrong with this approach?"_ That you are using raw pointers and `new` in c++ code. –  Jan 29 '18 at 18:00
  • *What is wrong with this approach?* -- You're trying to implement something that no inexperienced C++ programmer has ever implemented correctly. You'll be the first, if not one of the handful of new programmers to do this if you got a linked list to work correctly and without bugs. – PaulMcKenzie Jan 29 '18 at 18:01
  • 1
    A good coding style is one declaration per line. This would help remove defects like this one. – Thomas Matthews Jan 29 '18 at 18:05
  • @PaulMcKenzie Your comment is anything but useful. You made this rant and finally did not even answer. The SO community is to learn, not criticize. – FlyingAura Jan 29 '18 at 18:18
  • My answer would be a link to a linked list implementation that works correctly, easily understood, so that the OP can learn how to properly put one together. You see that the code posted makes use of uninitialized pointers as if they're initialized and other basic mistakes. – PaulMcKenzie Jan 29 '18 at 18:24
  • 1
    Unrelated: `typedef struct Node { int data; Node* next; } Node;` is unnecessary in C++. `struct Node { int data; Node* next; };` is all that is needed. Also remember to `delete` what you `new` or [take advantage of RAII](https://stackoverflow.com/questions/2321511/what-is-meant-by-resource-acquisition-is-initialization-raii) and use smart pointers. – user4581301 Jan 29 '18 at 19:18
  • @user4581301 Thanks! I had no clue that typedef could be omitted. Any reasons for that though? – user248884 Jan 29 '18 at 20:04
  • In C every time you used a `struct`, you had to say it was a `struct` or do that `typedef` trick to give it a different alias you could use. Whole bunch of possible reasons for this, being brutally explicit about exactly what the identifier is is one (Linux kernel has strict rules on `typdef`s that disallow `typedef`ing `struct` for this reason), but it could simply be in early C implementers didn't have a good way to leave it out. C++ is a different language with different rules, and one of those rules is you don't have to carry the `struct` around. – user4581301 Jan 29 '18 at 20:27

2 Answers2

4
Node *a, // not initialized
     *b, // not initialized
     *c, // not initialized
     *d, // not initialized
     *e, // not initialized
     *f, // not initialized
     *g = new Node(); // the only one initialized
O'Neil
  • 3,790
  • 4
  • 16
  • 30
2

Your problem is the following line:

Node *a,*b,*c,*d,*e,*f,*g = new Node();

This line declares the variable *a, *b, ... and *g. But you only initialize the variable *g. You would have to do the following:

Node *a = new Node(), 
     *b = new Node(),
     *c = new Node(),
     // and so on
     *g = new Node();
Benjamin J.
  • 1,239
  • 1
  • 15
  • 28