 |
 |
| View previous topic :: View next topic |
| Author |
Message |
noyz
Joined: 31 Jan 2009
Posts: 59
|
| Struct malloc and pointers problem |
 Posted: Fri Jun 17, 2011 12:09 pm |
 |
|
Hi all
i have the following :
| Code: |
typedef struct tnode {
struct tnode *prev;
int16 value;
struct tnode *next;
} myNode;
myNode *firstNode,*lastNode;
|
if i have in main :
| Code: |
myNode *nod = &firstNode;
nod->value = 200;
nod->next = malloc ( sizeof(myNode) );
nod = nod->next;
nod->value = 300;
nod->next = malloc ( sizeof(myNode) );
nod = nod->next;
nod->value = 500;
nod->next = NULL;
while (true) {
myNode *nd = &firstNode;
while (!isLastNode(nd)) {
lcd_gotoxy ( 5, 5, wTEXT );
lcd_Nr ( show, nd->value );
delay_ms ( 500 );
lcd_Nr ( hide, nd->value );
nd = nd->next;
}
}
|
the output is ok
taking in consideration that lcd nr and gotoxy works fine..
but i want to make a method for adding to my list.. myNode
so when i have :
| Code: |
void addToLast (myNode *nod, int16 value) {
//this node
nod->next = malloc ( sizeof(myNode) );
nod = nod->next;
//next nod
nod->value = value;
nod->next = NULL;
lastNode = &nod;
}
|
and then
| Code: |
addtolast(nod,100);
addtolast(nod,200);
addtolast(nod,300);
|
it doesn't work like it should be.. :(
how can i pass to arg the pointer needed ?
| Code: | | addtolast(&nod,100); // it outputs more strange than without & |
ccsc 4.120
pic 18f8527 |
|
 |
jeremiah
Joined: 20 Jul 2010
Posts: 1349
|
|
| Posted: Tue Jun 21, 2011 9:37 am |
|
|
A few things that I see at first glance:
1. firstNode and lastNode are both pointers that don't point to anything (at least you don't show us assigning them to something). You need to malloc some memory for them or set them to NULL (but don't use them until you malloc some memory).
2. The line
| Code: | | myNode *nod = &firstNode; |
is a no no. The & gives the address of the pointer. You just want the pointer. Do this (no &):
| Code: | | myNode *nod = firstNode; |
Save the & operator for when you want a pointer to point to a real object.
Now lets look at your function:
| Code: |
void addToLast (myNode *nod, int16 value) {
//this node
nod->next = malloc ( sizeof(myNode) );
nod = nod->next;
//next nod
nod->value = value;
nod->next = NULL;
lastNode = &nod;
}
|
You are setting a pointer to the address of a pointer again here. Take out the & operator in that last line. The variable nod is a pointer already. I think this is what is getting you.
You are also forgetting to set the prev element.
As an aside:
You can make that function a bit more solid:
| Code: |
void addToLast (int16 value) {
if(NULL == lastNode){ //First node allocation
firstNode = lastNode = malloc ( sizeof(myNode) ); //may have to break this into 2 lines.
lastNode->value = value;
lastNode->next = NULL;
lastNode->prev = NULL;
}else if(NULL == lastNode->next){
lastNode->next = malloc ( sizeof(myNode) );
lastNode->next->prev = lastNode; //set prev before the switch
lastNode = lastNode->next; //switch;
lastNode->value = value;
lastNode->next = NULL;
}else{
//This is an error condition...need to handle this somehow
}
}
|
Mind you, I haven't had a chance to test that out (I am sure someone here could point out a mistake), but just trying to give an idea. Using it this way means the variable lastNode will always have the new value.
I would also take it a step further and use a struct to define a list type containing the firstNode and lastNode pointers, then pass that into the function I showed, rather than using globals. |
|
|
noyz
Joined: 31 Jan 2009
Posts: 59
|
|
| Posted: Thu Jun 23, 2011 4:44 am |
|
|
please test this in main
| Code: |
myNode *firstNode, *lastNode;
myNode *nod = firstNode;
nod->prev = null;
nod->value = ( long ) 11;
nod->next = malloc ( sizeof(myNode) );
nod->next->prev = nod;
nod = nod->next;
nod->value = ( long ) 32;
nod->next = malloc ( sizeof(myNode) );
nod->next->prev = nod;
nod = nod->next;
nod->value = ( long ) 14;
nod->next = malloc ( sizeof(myNode) );
nod->next->prev = nod;
nod = nod->next;
nod->value = ( long ) 451;
nod->next = malloc ( sizeof(myNode) );
nod->next->prev = nod;
nod = nod->next;
nod->value = ( long ) 11231;
nod->next = NULL;
lastNode = nod;
printf ( "forward\r\n" );
myNode *nodulet = firstNode;
while (nodulet != NULL) {
printf ( "%Ld\r\n", nodulet->value);
nodulet = nodulet->next;
}
printf ( "backward\r\n" );
nodulet = lastNode;
while (nodulet != NULL) {
printf ( "%Ld\r\n", nodulet->value );
nodulet = nodulet->prev;
}
|
I've test it in Proteus.. and output is :
forward
217
241
253
265
277
backward
183
185
277
265
253
241
217
what is happening ?
I have 11, 32,14,451 and 11231... |
|
|
FvM
Joined: 27 Aug 2008
Posts: 2337
Location: Germany
|
|
| Posted: Thu Jun 23, 2011 9:10 am |
|
|
| Before doing any further tests, you absolutely have to correct the severe errors shown by jeremiah. You are writing a lot of data to nowhere. |
|
|
noyz
Joined: 31 Jan 2009
Posts: 59
|
|
| Posted: Thu Jun 23, 2011 9:43 am |
|
|
i can't quite explain why.. but this is ok
| Code: |
firstNode = NULL;
lastNode = NULL;
myNode *nod = &firstNode;
nod->prev = NULL;
nod->value = 1;
nod->next = malloc ( sizeof(myNode) );
nod->next->prev = nod;
nod = nod->next;
nod->value = 2;
nod->next = malloc ( sizeof(myNode) );
nod->next->prev = nod;
nod = nod->next;
nod->value = 3;
nod->next = malloc ( sizeof(myNode) );
nod->next->prev = nod;
nod = nod->next;
nod->value = 4;
nod->next = malloc ( sizeof(myNode) );
nod->next->prev = nod;
nod = nod->next;
nod->value = 5;
nod->next = NULL;
myNode *lst = malloc(sizeof(myNode));
lst = nod;
printf ( "forward %Ld \r\n", firstNode->value );
nod = &firstNode;
while (nod != NULL) {
printf ( "%Ld \r\n", nod->value );
nod = nod->next;
}
printf ( "backward %Ld \r\n", lastNode->value );
nod = &lst;
while (nod != NULL) {
printf ( "%Ld \r\n", nod->value );
nod = nod->prev;
}
nod = &lst->prev;
nod->next = malloc ( sizeof(myNode) );
nod->next->prev = nod;
nod = nod->next;
nod->value = 6;
nod->next = NULL;
lst = nod;
nod = &firstNode;
nod->prev = malloc ( sizeof(myNode) );
nod->prev->next = nod;
nod = nod->prev;
nod->value = -21;
nod->prev = NULL;
free (firstNode);
firstNode = malloc(sizeof(myNode));
firstNode = nod;
// free(lst);
// lst= malloc(sizeof (myNode));
printf ( "forward %Ld \r\n", firstNode->value );
nod = &firstNode;
while (nod != NULL) {
printf ( "%Ld \r\n", nod->value );
nod = nod->next;
}
printf ( "backward %Ld \r\n", lastNode->value );
nod = &lst;
while (nod != NULL) {
printf ( "%Ld \r\n", nod->value );
nod = nod->prev;
}
|
and output is :
| Code: |
forward 257
1
2
3
4
5
backward 0
0
5
4
3
2
1
forward -21
1
2
3
4
5
6
backward 0
0
6
5
4
3
2
1
-21
|
even so... the first node is strange value..
forward 257
but the first value written from it is ok:-?? |
|
|
noyz
Joined: 31 Jan 2009
Posts: 59
|
|
| Posted: Thu Jun 23, 2011 9:45 am |
|
|
are you refering to
| Code: |
firstNode= malloc(sizeof(myNode));
lastNode= malloc(sizeof(myNode));
|
?? |
|
|
noyz
Joined: 31 Jan 2009
Posts: 59
|
|
| Posted: Thu Jun 23, 2011 9:55 am |
|
|
please try my example in proteus..
and then write back..
it seems that is really strange this pointer thing in ccsc
write the whole working example.
thanks |
|
|
jeremiah
Joined: 20 Jul 2010
Posts: 1349
|
|
| Posted: Tue Jun 28, 2011 9:14 am |
|
|
Your first mistake I see is here:
| Code: |
firstNode = NULL;
lastNode = NULL;
myNode *nod = &firstNode;
|
You did good in initializing the pointers to NULL (good practice). However, you need to allocate memory for them before you use them. This is why you get the 257. You are pointing to random memory because you didn't allocate your own.
the line:
| Code: | | myNode *nod = &firstNode; |
will cause you problems because:
a) &firstNode is the address of the pointer firstNode
b) firstNode is NULL (not pointing to anything)
Try this instead:
| Code: |
firstNode = NULL;
lastNode = NULL;
myNode *nod;
nod = malloc(sizeof(myNode));
firstNode = nod;
lastNode = nod;
|
assuming I typed it correctly, this will allocate memory for your first node, which is something your current code is not doing at all.
It will then set firstNode and lastNode to point to that same node.
Also, you did something else wrong:
| Code: |
nod = &firstNode;
nod = &lst->prev;
|
This is a NO-NO. The variable firstNode is already a pointer and the same type as the variable nod. Do this instead:
| Code: |
nod = firstNode;
nod = lst->prev;
|
In your scenario, you shouldn't be using the & at all. It is used to get the address of a variable. If the variable is already a pointer, then you don't need the &.
Here is a 3rd problem, though you might not see it visually:
| Code: |
myNode *lst = malloc(sizeof(myNode));
lst = nod;
|
Using malloc is indeed important if the pointer doesn't have anything to point to. However, here you allocate memory and then immediately discard it. The memory you allocated with malloc is still there, but you no longer know where it is nor can use it because you set lst=nod. You have created what is called a "memory leak".
Memory that is allocated, that you no longer use should be "freed" using the free() function (but only once you are done).
At this point, I would say until you understand pointers, you shouldn't worry about getting this code to work as is. Take some time and study on how to use pointers in C (should be easy to google search for). Once you understand pointers, try again. |
|
|
|
|
You cannot post new topics in this forum
You cannot reply to topics in this forum
You cannot edit your posts in this forum
You cannot delete your posts in this forum
You cannot vote in polls in this forum
|
Powered by phpBB © 2001, 2005 phpBB Group
|
FAQ 
Search
Register
Profile 
Log in to check your private messages 
Log in
