Re: HELP, INFINIT LOOP... simple LINKED LIST
From: goose (ruse_at_webmail.co.za)
Date: 10/23/03
- Next message: Fergus Henderson: "Re: why still use C?"
- Previous message: pete: "Re: Best method to sort a linked list (in my case)?"
- In reply to: na1paj: "HELP, INFINIT LOOP... simple LINKED LIST"
- Next in thread: goose: "Re: HELP, INFINIT LOOP... simple LINKED LIST"
- Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]
Date: 23 Oct 2003 02:18:53 -0700
na1paj@yahoo.com (na1paj) wrote in message news:<e2c40fce.0310221847.32dafb3e@posting.google.com>...
> here's a simple linked list program. the DeleteNode function is
> producing an infinit loop i think, but i can't figure out where..
a really good idea would be to turn up the warning/diagnostics
level of your compiler. let your compiler catch the obvious
bugs. I've numbered my "fixes", this lets me only explain
each "fix" once, and refer back to it when I come across it again.
It would be a good idea if you wrote a function that could
loop though a list and print out each element. that way you
can verify that what you added to the list is in the list and
what you deleted from the list has in fact been deleted.
>
>
> #include <stdio.h>
1. you also need to #include <stdlib.h> for the malloc/free
function calls and string.h (for strncpy):
#include <stdlib.h>
#include <string.h>
> typedef struct
> {
> char *str; //str is a dynamic array of characters
> int length; //number of characters
> } String;
>
> typedef struct node
> { f struct node
> String Data;
> struct node* Link;
> } ListNode;
>
> typedef struct listStuct
> { f struct listStuct
> ListNode *Head;
> }List;
>
> void AddNode(List *L, String item)
> { ddNode(List *L, String item)
> ListNode *currNode, *newNode; ers
> currNode = L->Head; Node; ers
> if (L->Head == NULL) Node; ers
> { ULL) Node; ers
> L->Head = (ListNode*)malloc(sizeof(ListNode)); afb3e@posting.google.com>...
2. there is no need to cast the return of malloc. yuor compiler
probably complained about this line without the cast because
you didn't include stdlib.h.
3. a better idea is to use sizeof on the variable itself,
not on the type:
L->Head = malloc(sizeof L->Head); e itself,
4. you must *always* check the return of malloc. what in case
there was no memory ? do something like this:
if (L->Head==NULL) {
printf ("no memory for item %s\n", item.str);
return;
}
> L->Head->Data = item; .str);
5. there is a fundamental misconception here with regard to the
way you /think/ the String struct works. when you /assign/
item to L->Head->Data, the field "length" gets set correctly.
but the /other/ field ("str") does not get copied into. I
suspect that that is what you would want. you should do this
instead:
L->Head->Data.length = item.length;
L->Head->Data.str = malloc (item.length);
if (L->Head->Data.str==NULL) {
printf ("no memory for str%s\n", item.str);
free (L->Head);
return;
}
strncpy (L->Head->Data.str, item.str, item.length);
L->Head->Data.str[item.length] = '\0';
> L->Head->Link = NULL; ;
> } L->Head->Link = NULL; ;
> else L->Head->Link = NULL; ;
> { >Link = NULL; ;
> while(currNode->Link != NULL)
> currNode = currNode->Link; //go to the last afb3e@posting.google.com>...
> newNode = (ListNode*)malloc(sizeof(ListNode)); afb3e@posting.google.com>...
see fix #2, #3 and #4:
newNode = malloc(sizeof *newNode); ode));
afb3e@posting.google.com>...
if (newNode==NULL) {
printf ("no memory for item %s\n", item.str);
return;
}
> newNode->Data = item; .str);
see fix #5:
newNode->Data.length = item.length;
newNode->Data.str = malloc (item.length);
if (newNode->Data.str==NULL) {
printf ("no memory for str%s\n", item.str);
free (newNode);
return;
}
strncpy (L->Head->Data.str, item.str, item.length);
L->Head->Data.str[item.length] = '\0';
> newNode->Link = NULL; ngth);
> currNode->Link = newNode; ngth);
> }
> }
>
> void DeleteNode(List *L, String item)
> { eleteNode(List *L, String item)
> ListNode *currNode, *prevNode; ; ngth);
> currNode = L->Head;
> prevNode = L->Head; vNode; ; ngth);
> if (L->Head == NULL)
> { ULL)
> printf("empty list"); ; ngth);
6. you have correctly identified this as an empty list.
dont you think it might be better to merely return
immediately ? there is no need to actually check,
because the loop below will never execute if the
list is empty.
> } .
> while(currNode != NULL) //check if first ode)); afb3e@posting.google.com>...
> { //check if first ode)); afb3e@posting.google.com>...
> if(strcmp(currNode->Data.str,item.str)==0) //found sting.google.com>...
> {
> if(currNode == L->Head) //first one
> {
> L->Head == currNode->Link;
7. this line does nothing. your compiler might have emitted
a diagnostic (did your compiler complain?). I replaced it
withs:
L->Head = currNode->Link;
> currNode->Link = NULL;
> free(currNode);
8. before you can do that, we need to free the memory in item
that I allocated earlier. do this instead:
free (currNode->Data.str);
free (currNode);
> printf("Remove First");
> }
> else
> { item
> prevNode->Link = currNode->Link; >...
> currNode->Link = NULL;
> free(currNode); >...
see #8 above:
free (currNode->Data.str);
free (currNode);
> printf("removed ");
> } removed ");
> }
> else
> {
> prevNode = currNode;
> currNode = currNode->Link; //go to next one >...
> }
> }
> }
>
>
> int main()
> { in()
> FILE *fp = NULL; = currNode->Link; //go to next one >...
> List list; = currNode->Link; //go to next one >...
> String string; = currNode->Link; //go to next one >...
> int i; tring; = currNode->Link; //go to next one >...
> int records = 0; = currNode->Link; //go to next one >...
> char filename[100];
> //list.Head = (ListNode*)malloc(sizeof(ListNode)); >...
> list.Head = NULL; istNode*)malloc(sizeof(ListNode)); >...
> string.str = (char *)malloc(sizeof(char) *20); istNode)); >...
9. sizeof (char) is always equal to one, also see fixes
#2 and #4 above:
string.str = malloc(20); to one, also see fixes
if (string.str==NULL) {
printf ("no memory\n");
return EXIT_FAILURE;
}
> printf("Enter filename\n"); also see fixes
> scanf("%s", filename); "); also see fixes
> fp = fopen(filename, "r"); also see fixes
> fp = fopen(filename, "r"); also see fixes
> if (fp == NULL)
> { printf ("Error\n"); >...
> exit(0); printf ("Error\n"); >...
> }
> else
> {
> fscanf(fp, "%d", &records);
> for (i = 0; i<records; i++) "); >...
> {
> fscanf(fp, "%s", string.str);
10.what in case the string you are trying
to read in is bigger than the space you allocated ?
try this instead:
fscanf(fp, "%19s", string.str);
11.you forget to set the length field of the struct
"string", try this:
string.length = strlen (string.str) +1;
> AddNode(&list, string);
> printf("%s ", string.str);
> }
> printf("\n\nEnter a Node to delete: ");
> scanf("%s", string.str);
> DeleteNode(&list, string);
> }
> return 1;
> }
hth
goose,
hand
- Next message: Fergus Henderson: "Re: why still use C?"
- Previous message: pete: "Re: Best method to sort a linked list (in my case)?"
- In reply to: na1paj: "HELP, INFINIT LOOP... simple LINKED LIST"
- Next in thread: goose: "Re: HELP, INFINIT LOOP... simple LINKED LIST"
- Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]
Relevant Pages
|
