2

I am new to programming.The other day I was playing around with structures and pointers...I was getting errors.I tried to rectify them.It got rectified...But I cant justify why there was an error at the first place.Please help me...

struct node{
   int data;
   struct node*next;    
};  

int main(){
   struct node *newnode=NULL;
   struct node *start=NULL;
   newnode=start=(struct node*)malloc(sizeof(struct node));
   newnode->data=1;

   //code snippet 
   newnode->next=NULL;
   newnode=newnode->next;
   newnode=(struct node*)malloc(sizeof(struct node));
   newnode->data=2;
   start=start->next;//error probably as start->next is perceived as NULL Address
   printf("%d",start->data);
   return 0;    
}

when replacing code snippet with this code

newnode->next=(struct node*)malloc(sizeof(struct node));
newnode=newnode->next;
newnode->data=2;
start=start->next;
printf("%d",start->data);

error dissapers..How does one justify this ?

Barmar
  • 741,623
  • 53
  • 500
  • 612
  • 1
    [Don't cast the return of `malloc`](https://stackoverflow.com/questions/605845/do-i-cast-the-result-of-malloc?utm_medium=organic&utm_source=google_rich_qa&utm_campaign=google_rich_qa) – Tormund Giantsbane May 25 '18 at 19:45
  • Note that it is unnecessary (but not harmful) to initialize your pointers to NULL when the very next thing you do is assign different values to them. – John Bollinger May 25 '18 at 19:49
  • As John says, just directly initialize your pointers if that's literally the next thing you were going to do anyway. Keeps your code more minimal and looks more consistent. – tadman May 25 '18 at 20:48

5 Answers5

2

You are overwritting the address of newnode here

newnode = newnode->next;

You probably want:

start = malloc(sizeof(struct node));
start->data = 1;

newnode = malloc(sizeof(struct node));
newnode->data = 2;
newnode->next = NULL;

start->next = newnode;

printf("%d", start->data);
David Ranieri
  • 39,972
  • 7
  • 52
  • 94
  • He knows how to fix it, his second version did that. He wanted to know why the first version was wrong. – Barmar May 25 '18 at 19:59
  • @Barmar, well, in the second version OP is doing the same error: `start=start->next;` (overwrites the value) – David Ranieri May 25 '18 at 20:01
  • But it doesn't cause a problem because `start->next` points to a valid node. In the first version `start->next` was a null pointer. – Barmar May 25 '18 at 20:03
  • @Barmar, I don't think so: `newnode=start=(struct node*)malloc(sizeof(struct node));` , the result of `malloc` is assigned to both pointers, anyway, it is not clear to me what OP is trying to do. – David Ranieri May 25 '18 at 20:04
  • But then he does `newnode->next=(struct node*)malloc(sizeof(struct node));`. – Barmar May 25 '18 at 20:07
  • That also assigns to `start->next`, so `start = start->next;` moves it to the second node. – Barmar May 25 '18 at 20:08
  • Sorry but I don't get you: as far as I can see, in the first version: `start = start->next;` overwrites the value returned by `malloc` with `NULL` (not with a valid address) – David Ranieri May 25 '18 at 20:15
  • That's correct. That's why the first version failed. The second one fixed this by assigning to `newnode->next` *before* doing `newnode = newnode->next`. – Barmar May 25 '18 at 20:18
  • Read my answer, it goes into great detail about what was wrong in the first version and why it works in the second. – Barmar May 25 '18 at 20:18
  • Ok, now I get you, and yes, your answer makes clear where the error is. – David Ranieri May 25 '18 at 20:22
1

In the first code, newnode and start both start off pointing to the same node that you allocated. You then set newnode->next = NULL;, so that also sets start->next to NULL.

You then do:

newnode = newnode->next;
newnode = (struct node*)malloc(sizeof(struct node));

so newnode now points to a different node, but start still points to the original node. So start->next is still NULL. Then you do:

start = start->next;

This sets start to NULL, so trying to print start->data is invalid because you can't dereference a null pointer.

The first assignment before malloc() is pointless, because you're immediately replacing the variable with something else. I'm guessing you thought that by first reassigning newnode, the malloc() call would update both newnode and newnode->next because you've declared them to be equivalent. But that's not how assignments work -- all it does is copy the value of newnode->next into newnode, it doesn't link those two places together.

Your second version gets this right by assigning to newnode->next rather to newnode. This also assigns to start->next because newnode and start initially point to the same structure. Then you assign

newnode = newnode->next;

This updates newnode, but start is still OK and points to the first node. When you then do

start = start->next;

it updates start to point to the second node as well. In this case, start->data is valid, it contains the same thing that you assigned to newnode->data a few lines earlier.

Barmar
  • 741,623
  • 53
  • 500
  • 612
0

In the first of your two codes, you have

newnode=newnode->next;
newnode=(struct node*)malloc(sizeof(struct node));

. The first of those assignments is useless: whatever value is thereby assigned to newnode is in the next line replaced by a different one. Moreover, nothing is assigned to the next member of the structure to which newnode initially pointed. The first assignment to newnode does not somehow bind it to that next pointer, as an alias for the pointer itself. It just makes newnode point to the same thing.

In the second code, you instead have:

newnode->next=(struct node*)malloc(sizeof(struct node));
newnode=newnode->next;

That makes a lot more sense. You first allocate memory and assign newnode->next to point to it, then you update newnode to point to the new memory, too. Furthermore, up to that point you still have start pointing to the first dynamically-allocated block, so you can still access that, and since newnode and start initially pointed to the same structure, the same value can afterward be obtained via the expression start->next, too.

John Bollinger
  • 160,171
  • 8
  • 81
  • 157
0

Let me comment what the first version does:

newnode=start=(struct node*)malloc(sizeof(struct node));
...
// at this point, newnode and start point to the same node

newnode->next=NULL; 
// since start points to the same node as newnode, this means that also 
// start->next will be NULL 
...
start=start->next; // since start->next is null, start will be null after this statement

printf("%d",start->data);  // start is null, so start->null dereferences NULL (undefined behaviour, e.g. a crash or something dubious)

Your second version is different, because...

newnode->next=(struct node*)malloc(sizeof(struct node));
// this lets newnode->next point to a valid (new) node
// since start==newnode, also start->next will point to this valid (new) node

start=start->next;  // afterwards, start will point to the valid (new) node
printf("%d",start->data); // OK; should print 2 then. 

I think you might confuse an assignment expression with "defining an alias":

newnode=newnode->next;
newnode=(struct node*)malloc(sizeof(struct node)); 

is very different from

newnode->next = (struct node*)malloc(sizeof(struct node));

because

newnode = newnode->next 

is not defining a macro or a replacement text for newnode such that text newnode is replaced with text newnode->next henceforth. It's rather that the variable newnode will get a new value, i.e. the value that member next of this node points to.

Stephan Lechner
  • 34,891
  • 4
  • 35
  • 58
0

You are correct that the error comes from printf("%d",start->data); where you are trying to dereference a pointer to NULL.

Let me explain it graphically: with the firs 4 lines you get this:

             _____________
newnode ->  | data = 1    |
start   ->  | next = null |
            |_____________|

This line newnode->next=NULL; does nothing. Then, newnode=newnode->next; does this:

             _____________
            | data = 1    |
start   ->  | next = null |      newnode -> NULL
            |_____________|

Then,

 newnode=(struct node*)malloc(sizeof(struct node));
 newnode->data=2;`

does this:

             _____________                    _____________
            | data = 1    |                  | data = 2    |
start   ->  | next = null |      newnode ->  | next = null | 
            |_____________|                  |_____________|

Finally, start=start->next; sets the state like this, and clearly you try to access a field of a pointer to NULL as a result.

                                              _____________
                                             | data = 2    |
start   ->  null                 newnode ->  | next = null | 
                                             |_____________|
samuelnj
  • 1,627
  • 1
  • 10
  • 19
  • Ok i kind of get in now..Thank you for the answer..Was quite Helpful!!! –  May 26 '18 at 05:37